Event Handlers 2.0 (Event.py)

[thiell]

Worker and Task classes are becoming too complex and not flexible enough, we need to move some of the features managed there to event handlers. For example, feature #231 should be implemented using extended event handlers described in this ticket.

Indeed the original event handling mechanism is not flexible enough, so it must itself be changed. Some of the current limitations of Event.EventHandler are:

• access to node, msg or rc are done through Worker.current_ accessors instead of directly accessible method arguments
• the API do not support named Engine I/O streams (d3e23a6b0f8129118df09568c411a5eef1968117)
• ...

My proposal is to define a new module named Event2.py defining a new EventHandler base class.

Changes Event2.EventHandler vs. Event.EventHandler:

• ev_error is merged with ev_read which becomes "named stream aware"
• ev_read and ev_hup get a full arguments list (other idea is to use **kwargs but doesn't seem to be the more convenient solution):
  • def ev_read(self, worker, node, fname, msg):
  • def ev_hup(self, worker, node, rc):

Change in Worker:

• Worker detects the use of Event2.EventHandler and delegates further work to it, otherwise (V1) there is no change
• when no handler is assigned to a Worker, use DefaultEventHandler which supports MsgTree management (compat) CHANGED: DefaultEventHandler is indeed Event2.EventHandler

Change in user-programming:

• in Event2.EventHandler, parents method should be called if associated feature is wanted; this must be clearly documented
• no change at all if user is still using Event.py

Included EventHandler V2:

• DefaultEventHandler (concrete) as a compat EventHandler (Task's stdout/stderr MsgTree support) CHANGED: DefaultEventHandler is indeed Event2.EventHandler
• see if some of CLI/Clush.py could be generalized or a ProgressEventHandler ?

[work in progress]

[thiell]

Ideas for EventHandler v2 method signatures:

ev_start(self, worker)
ev_pickup(self, worker, node)
ev_read(self, worker, node, sname, msg)
ev_written(self, worker, node, sname, count)
ev_hup(self, worker, node, rc)
ev_timeout(self, worker, nodeset/nodelist?)
ev_close(self, worker)

and ev_error

Some thoughts:

• arguments order? Is putting sname after node better in practice? as sname is probably less used and more for advanced use. But would (worker, sname, node) be more coherent in theory?
• ev_hup is only valid when the worker is bound to a real command (that returns an exit status), so it's not as generic as the others
• we could remove ev_timeout and add parameters to ev_close (either just a boolean "did_timeout" or directly the nodeset/nodelist of timeout'd nodes ?)

Note: there are also the following two methods, but not worker related:

ev_msg(self, port, msg)
ev_timer(self, timer)

Let me know what you think about it!

[degremont]

• I'm fine with argument order.
• ok for dropping ev_error.
• ev_hup: what are the use cases when it is not called?
• ok for removing ev_timeout, it is rather useless. I never use it. I'm always using did_timeout() in ev_close(). I do not know what is the good prototype though, DistantWorker/StreamWorker do not have the same info (no nodeset for StreamWorker)

Important point is getting rid of current_ stuff which is very context dependent. pickup, read, written, hup are the important ones.
For ev_close, accessors like did_timeout() will not change its value anymore for this worker.

• ev_timer and ev_msg look good.

By the way, a stupid idea: do we need to use better names for some callbacks? start, read and written are really understandable without having to look at the doc. Is that ok for the others?

[thiell]

Ok perfect.

• ev_hup is not called if your worker is a StreamWorker for example (bound to a stream, not a process)
• as for the names, I would be in favor of keeping the current ones, but we can still discuss that if you have great ideas (no rush)

[degremont]

Could we implement this using the same trick we used for ev_written() ? Only 4 callbacks need to be updated. If I'm correct, all the other ones are already good.

[thiell]

A note about the "trick" that we currently use at

https://github.com/cea-hpc/clustershell/blob/master/lib/ClusterShell/Worker/Worker.py#L164

This is deprecated in Python 3 and I don't think any of the recommended methods is available in Python 2. Adding another "if" on that part of the code might not be a good idea. Of course we can stick with the deprecated method for now and ignore the warnings.

test write + worker.abort() on read to reuse FDs ... /home/travis/build/cea-hpc/clustershell/lib/ClusterShell/Worker/Worker.py:694: DeprecationWarning: inspect.getargspec() is deprecated, use inspect.signature() or inspect.getfullargspec()

[thiell]

ok n/m, I think we can do a smart py2/py3 import thanks to getfullargspec() in Python 3, see the Python doc:

inspect.getfullargspec(func)
Changed in version 3.6: This method was previously documented as deprecated in favour of signature() in Python 3.5, but that decision has been reversed in order to restore a clearly supported standard interface for single-source Python 2/3 code migrating away from the legacy getargspec() API.
from https://docs.python.org/3/library/inspect.html#inspect.getfullargspec

That will be so cool to use an undeprecated method!