on_open_error_callback not called for ProbableAuthenticationError

[awelzel]

Hi Gavin,

I don't have much insight into all the interactions of the code, but the following seems to do the right thing for the TornadoConnection and should not change the existing behavior (except for calling the on_open_error_callbacks).

Thoughts?

Thanks,
Arne

[awelzel]

#525

[coveralls]

Coverage decreased (-3.36%) to 69.26% when pulling 18de5f8 on awelzel:errorcb into 409670b on pika:master.

[coveralls]

Coverage increased (+0.02%) to 72.64% when pulling 18de5f8 on awelzel:errorcb into 409670b on pika:master.

[coveralls]

Coverage increased (+0.02%) to 72.64% when pulling 18de5f8 on awelzel:errorcb into 409670b on pika:master.

[gmr]

Have you examined the impact that your change to BaseConnection will have on all of the connection adapters?

[vitaly-krugl]

Is the exception being intentionally suppressed here? Please add a comment explaining why the exception is being suppressed and how user code might become informed of the failure.

[awelzel]

@vitaly-krugl , thanks for checking. The commit message had the reason:

tornado_connection: silence AMQPConnectionError in _adapter_disconnect
It's not possible to handle this exception anywhere and it's super noisy.
18de5f8

AFAICT, if a AMQPConnectionError happens at that point inside a tornado callback, you as a user can't catch it anywhere reasonably well and it.

However - I recently came across the following messages which seem to be caused by this change (in the scenario of #525 ):

CRITICAL:pika.adapters.base_connection:Tried to handle an error where no error existed

Seems some other code is relying for the exception to happen so it's not executed... :-/

[vitaly-krugl]

I can see how an exception here is a problem for frameworks like Tornado and Twisted that rely on continuation-passing style. I am not aware of an ideal solution for this in the existing pika code. Does TornadoConnection's stop_ioloop_on_close=True cause the ioloop to stop in the #525 scenario? I see that there are other types of exceptions that would be problematic for these frameworks, such as exceptions.ProtocolVersionMismatch.

[awelzel]

For torando I think only after applying b999636 - the code of the finally clause was skipped previously.

[vitaly-krugl]

I wonder if the following might do the trick for you (in addition to suppressing AMQPConnectionError in tornado_connection.py):

1. Add try/finally in BaseConnection._handle_disconnect():

     def _handle_disconnect(self):
         """Called internally when the socket is disconnected already
         """
         try:
             self._adapter_disconnect()
         finally:
             self._on_connection_closed(None, True)
   

2. Pass on_close_callback in addition to on_open_error_callback callback function to the TornadoConnection constructor.

I think this will assure that your on_close_callback will be called if AMQP handshake fails. The on_open_error_callback callback should be called if connection setup (TCP/IP connection or SSL handshake) fails prior to AMQP handshake.

on_open_error_callback is defined in pika as "Method to call if the connection cant be opened", which makes it seem like it's intended to report errors in TCP/IP connection establishment; whereas on_close_callback is intended for the AMQP handshake phase and later, since on_close_callback may be given the reason_code and reason_text args that can only come from the broker.

[awelzel]

I used on_open_error_callback because at that point, the state of the connection is not in CONNECTION_OPEN yet. Also, I think you do not get a reason/message from the broker. The connection essentially times out. That's why ProbableAuthenticationError.

I leave this decision to @gmr .

[vitaly-krugl]

@awelzel, the reason I proposed the try/finally BaseConnection._handle_disconnect() fix above is that it doesn't alter the documented semantics of on_close_callback and on_open_error_callback, shouldn't negatively impact any other connection classes, and is also deterministic versus "If the connection is neither open, not closed, it probably was never successfully established".

CC @gmr

[awelzel]

Have you examined the impact that your change to BaseConnection will have on all of the connection adapters?

Not in too close detail as they all use _adapter_disconnect() from the base connection. I have tested the SelectConnection and confirmed it is still working and will indeed call on_open_error_callback when provided.

A possibly user visible change is that for errors that happen after adapter_connect(), but before _on_connection_open(), a call to the provided on_open_error_callback will now be done for all adapters (see below for twisted). But, that seems to be the right thing to do and the reason for #525 to begin with.

Further, _handle_ioloop_stop and _init_connection_state will be properly called in above scenario.

The default implementation (_on_connection_error) will simply raise an exceptions.AMQPConnectionError as well, so this will keep the behavior.

The twisted connection uses its own _adapter_disconnect() so it's not affected. However, I think it has the same problem - on_open_error_callback won't be called for "probably auth" errors.

[vitaly-krugl]

The title of the PR refers to ProbableAuthenticationError, but the exception being trapped here is more general.

[awelzel]

The title of the PR refers to ProbableAuthenticationError, but the exception being trapped here is more general.

Yes, that is true. I observed the problem only with the ProbableAuthenticationError. IMO, however, the on_open_error_callback should be called for any connection errors in cases where the connection has not been fully opened.

[vitaly-krugl]

@awelzel, it looks like the issue has been addressed by the merged PR #672. Could you please close this PR? Thx!

CC @gmr

[awelzel]

@vitaly-krugl , I have tested that PR #672 fixes the issue. This PR is no longer relevant.

[vitaly-krugl]

Thank you.