Exception handling too broad, interferes with GeventExit and SystemExit

[vitaly-krugl]

In a couple of places, haigha's exception handling is too broad, resulting in undesirable behavior for system-level exceptions such as SystemExit and GeventExit. Think of GeventExit and SystemExit like "kill -9".

Instead of "except:", these should be handling specific error classes that inherit from Exception. Ideally, these should be "except specific-expected-error-classes:", and at the minimum they should be "except Exception:", but never just "except:".

in haigha/channel.py, see the "except:" block below:

  def process_frames(self):
    '''
    Process the input buffer.
    '''
    while len(self._frame_buffer):
      try:
        # It would make sense to call next_frame, but it's technically faster
        # to repeat the code here.
        frame = self._frame_buffer.popleft()
        self.dispatch( frame )
      except ProtocolClass.FrameUnderflow:
        return
      except:
        # Spec says that channel should be closed if there's a framing error.
        # Unsure if we can send close if the current exception is transport
        # level (e.g. gevent.GreenletExit)
        self.close( 500, "Failed to dispatch %s"%(str(frame)) )
        raise

And in haigha/connection.py:

  def disconnect(self):
    '''
    Disconnect from the current host, but do not update the closed state. After
    the transport is disconnected, the closed state will be True if this is 
    called after a protocol shutdown, or False if the disconnect was in error.

    TODO: do we really need closed vs. connected states? this only adds 
    complication and the whole reconnect process has been scrapped anyway.

    '''
    self._connected = False
    if self._transport!=None:
      try:
        self._transport.disconnect()
      except: 
        self.logger.error("Failed to disconnect from %s", self._host, exc_info=True)
        raise
      finally:
        self._transport = None

[awestendorf]

As noted in the comments, the goal is to ensure that the protocol is adhered to while also re-raising the exception. The exceptions that you note should be reraised. Can you provide more detail on when and how that path fails?

[vitaly-krugl]

Regarding "Spec says that channel should be closed if there's a framing error.", exceptions like SystemExit are not framing errors. SystemExit and friends are specifically not derived from Exception so that applications/libraries will just let them percolate unfettered. As far as I know, we should normally not be catching exceptions that do not inherit from Exception.