jaraco / irc

Full-featured Python IRC library for Python.
MIT License
392 stars 86 forks source link

client crashes on EINTR #36

Closed jaraco closed 8 years ago

jaraco commented 8 years ago

I have been using irclib to build an irc bot daemon, but after I handle any signal, process_forever of client.py raises an InterruptedSystemCall exception. I traced the source down to the select.select statement in process_once. The select() system call always returns EINTR whenever it's interrupted by any signal (even ignoring siginterrupt), and for some reason the Python select library chose not to handle this by retrying, instead simply reraising the exception for the user.

There's nothing for an irclib user to do but to let process_forever die and restart the entire client (or maybe there's some weird hack around it?), but this is a fixable issue in the library if the select.select is wrapped in something like:

#!python

while 1:
  try:
    (i, o, e) = select.select(sockets, [], [], timeout)
    break
  except select.error as e
    if e[0] != errno.EINTR: raise

jaraco commented 8 years ago

It's not obvious to me that the right thing to do is simply suppress EINTR, though I'm not particularly familiar with when that signal would be raised or what the proper thing is to do in that case. However, since the release of 8.6, it's now possible to customize the Manifold class (formerly IRC class) and customize the process_forever behavior. Consider:

class RetryOnEINTRManifold(Manifold):
    def process_once(self, *args, **kwargs):
        """
        Override process once to suppress EINTR signals.
        """
        try:
            return super(RetryOnEINTRManifold, self).process_once(*args,
                **kwargs)
        except select.error as ex:
            if ex[0] != errno.EINTR:
                raise

ServerConnection.manifold_class = RetryOnEINTRManifold

Give that a try and let me know how it goes. If it works for you, but you still believe this behavior is something that all clients should expect, then please do provide some explanation or justification so we can have better documentation for the change.


Original comment by: Jason R. Coombs

jaraco commented 8 years ago

I'm going to mark this as resolved for now, but do please feel free to follow-up, especially if the workaround above does not work as prescribed.


Original comment by: Jason R. Coombs