agoragames / haigha

AMQP Python client
BSD 3-Clause "New" or "Revised" License
160 stars 41 forks source link

Diagnostic info loss or app failure results when connection is closed by broker #12

Closed vitaly-krugl closed 12 years ago

vitaly-krugl commented 12 years ago

Channel._closed_cb() sets various member variables to None, such as self.channel. If you later try to check if the channel is closed via ch.closed (which translates in ch.channel.closed) or ch.close_info (which translates into ch.channel.close_info), you get one of those "AttributeError("'NoneType' object has no attribute..." exceptions.

On a related note, if you you registered a channel-close callback via Channel.add_close_listener(), then the listener callback gets called after self.channel and friends are set to None, so ch.close_info (that you would want to log) also causes the above-mentioned exception.

I could do something ugly, like save a reference to the channel's internal channel member immediately after creating the channel so that I can access close_info, etc. later, but that has the usual pitfalls of messing with the internals of a class.



  def _closed_cb(self, final_frame=None):
    if final_frame:
      self._connection.send_frame( final_frame )
    self._pending_events = deque()
    self._frame_buffer = deque()

    # clear out other references for faster cleanup
    for protocol_class in self._class_map.values():
      protocol_class._cleanup()
    self._connection = None
    self.channel = None
    self.exchange = None
    self.queue = None
    self.basic = None
    self.tx = None
    self._class_map = None

    for listener in self._close_listeners:
      listener( self )
    self._close_listeners = set()

awestendorf commented 12 years ago

Good stuff. The "set to None" is there to help reference counting clean up channels under high load without relying on the garbage collector. I'll work on fixing this.

awestendorf commented 12 years ago

I think a good way to way to solve this would be to restructure the code in a try-finally block and document clearly that the listeners will be able to access the channel attributes at call time but they'll be out of scope after that.

The close listeners are brand new as I needed them to fix a variety of problems in the ChannelPool so please keep writing up bugs for anything associated with them.

  def _closed_cb(self, final_frame=None):
    if final_frame:
      self._connection.send_frame( final_frame )

    try:
      for listener in self._close_listeners:
        listener( self )
    finally:
      self._pending_events = deque()
      self._frame_buffer = deque()

      for protocol_class in self._class_map.values():
        protocol_class._cleanup()

      self._connection = None
      self.channel = None
      self.exchange = None
      self.queue = None
      self.basic = None
      self.tx = None
      self._class_map = None
      self._close_listeners = set()
vitaly-krugl commented 12 years ago

That looks very reasonable. I think that this also needs to be complimented with a small change in Channel class that allows the user to access the ch.closed attribute at any time without crashing. Perhaps something like this:


  @property
  def closed(self):
    return self.channel is None or self.channel.closed

# Usage example
if not ch.closed:
    ch.close()
vitaly-krugl commented 12 years ago

The user would also need to know that their last opportunity to extract close_info is in the close-listener callback. However, in my code, I also wished I could to do something like this from anywhere:

if ch.closed:
    self._logger.error("Channel closed unexpectedly; close_info: %r", ch.close_info)
awestendorf commented 12 years ago

Fixed this in master by moving all channel state into Channel and out of ChannelClass. Will go out with the next release later today.