IOLoop subclasses #673

Closed
minrk opened this Issue Jan 24, 2013 · 6 comments

Comments

Projects
None yet
2 participants
Contributor

minrk commented Jan 24, 2013

I maintain zeromq/pyzmq, which ships a copy of tornado.ioloop (as of 2.4.1), and I've been looking into the changes coming for tornado 3.0, and what they might mean for pyzmq.
The changes made for zmq compatibility are minimal:

  1. IOLoop.close(socket) adds a check for fd.close(), so that it can close zmq sockets. This is easy enough with a subclass, since the close method is small.
  2. poll() can raise particular errors. base IOLoop handles EINTR specially, but I add a switch to interpret zmq.ETERM as an instruction to exit the loop. It wouldn't be awesome to override start(), because it's a tiny change in a massive method.

My question is, would you be amenable to making it easier to add this ETERM functionality? Either via config (e.g. IOLoop.errno_handlers[zmq.ETERM] = loop.stop) or breaking up loop.start() into smaller bites (e.g. try: poll(); except: self._handle_poll_error())?

Owner

bdarnell commented Apr 14, 2013

I'm sorry I let this fall through the cracks before the 3.0 release. It would be fine with me to call fd.close() if it exists instead of os.close(fd) (I think this also helps with jython), but it looks like that's no longer in pyzmq. Do you still need it?

For zmq.ETERM, could you put that in the poller itself (the EINTR checker arguably belongs in the pollers too)? I guess the problem is that the poller doesn't have a reference to the IOLoop (and must be constructed first). I think that what you have now isn't so bad; I'd like to see another example besides stopping the IOLoop before we try and devise a more sophisticated error-handling scheme.

Contributor

minrk commented Apr 15, 2013

it looks like that's no longer in pyzmq. Do you still need it?

Yup - I missed that part when I updated the shipped loop to 3.0. I think a simple

try:
    fd.close()
except AttributeError:
    os.close(fd)

would do it for this one (or whatever style you guys prefer in terms of capability checking).


For the error handling, I think that EINTR handing definitely belongs in the poller itself, that's a great idea. ETERM is different, because it should signal loop stop. I see two basic ways to do this without adding extensible error handling to IOLoop:

  1. give the Poller a handle on the loop itself, so it can just call loop.stop()
  2. define a special IOLoopShouldStop exception, that the poller could raise, which prevents the loop from continuing.
Contributor

minrk commented Apr 15, 2013

I opened #735 for the close method

Contributor

minrk commented Apr 15, 2013

You know what, since ETERM should actually stop the loop, I can just do this in the zmq subclass:

def start():
    try:
        super(...).start()
    except ZMQError as e:
        if e.errno == ETERM:
            return
        else:
            raise

So I don't think you guys need to add that 'should exit' hook, unless you think it will be more generally useful.

Owner

bdarnell commented Apr 16, 2013

That's what you're already doing for the ETERM case, right? That seems reasonable, so let's leave it at that unless there is a second PollIOLoop-based implementation that needs it (in which case an IOLoopShouldStop exception seems like the way to go). Or now that I think about it, the poller could just use IOLoop.current().add_callback(IOLoop.current().stop)

bdarnell closed this Apr 16, 2013

Contributor

minrk commented Apr 16, 2013

Great, thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment