AutorecoveringConnection fires client shutdown hooks after connection has recovered

[doswell]

Version - amqp-client-3.6.1

When init is called on AutorecoveringConnection it adds the automatic recovery listener to the shutdownHooks list - Because this is the first entry, any user added shutdown hooks will be called after the automatic recovery.

as a possible solution;
The listener created in addAutomaticRecoverListener would likely need to call each shutdownListener, and omit adding the created listener to the shutdownHooks.
addShutdownListener, removeShutdownListener would only apply to the shutdownHooks list and not add to the delegate.
recoverShutdownListeners would just call addAutomaticRecoverListener.

[michaelklishin]

I'd like to see if this is in line with the idea of #132 first.

[michaelklishin]

Also note that shipping this in a 3.6.x release is probably unrealistic.

[doswell]

I think this fix allows for #132 to be easily done without other API
changes. As now you will get the shutdown listener fired when recovery
starts.
As a side note; Perhaps in terms of the auto recovery connection
shutdownlistener is a slightly misleading name vs disconnect listener.

Currently, if you add a shutdown listener which just logs, and are
connecting to one broker (I suspect if you use a cluster this is less
noticeable as the reconnect happens quickly to another member, so if
logging disconnects it seems correct);

Client Connect to server
Add shutdown listener and recovery listener to log status
(disconnect/connected)
Stop rabbitmq
-- no log output
Wait 5mins

Start rabbitmq
Client reconnects
-- logs output saying connected
-- logs output saying disconnected(for disconnect 5mins ago)

Funny enough the logging order was what I have seen from my tests; I think
the recovery listener ends up being fired before letting the remaining
shutdownlisteners run.

On Tue, Mar 8, 2016, 5:48 PM Michael Klishin notifications@github.com
wrote:

Also note that shipping this in a 3.6.x release is probably unrealistic.

—
Reply to this email directly or view it on GitHub
#135 (comment)
.

[michaelklishin]

@doswell ok, thanks for the explanation, the latter point is definitely convincing.

[michaelklishin]

@garyrussell @artembilan @spatula75 do you folks have an opinion on this change? It's breaking but makes sense to me. There is a pull request in #137.

[artembilan]

@michaelklishin ,
thank you for letting to know!

I would like to see the direct code changes to make a decision. Right now it is difficult to dig over all those GH cross links.
But from other side I believe @garyrussell in his comment: #132 (comment).
So, we really may live silently in Spring AMQP with that patch.

[spatula75]

Just want to make sure I understand the sequence of events that will happen so I can make an intelligent comment.

It sounds like it would go a bit like this (assuming I've declared a RecoverableConnection):

1. Connection dies
2. ShutdownListener is called
3. Recovery begins
4. Recovery completes, connection is good again
5. RecoveryListener is called

Is that correct?

[spatula75]

(If so, I think it would obsolete my PR, provided that we could tell somehow in step 2 above that recovery will be attempted.)

[michaelklishin]

@artembilan @garyrussell @spatula75 currently shutdown listeners are executed (effectively) concurrently with recovery which is kicked off by another shutdown listener. This PR makes the client first execute all shutdown listeners and then begin recovery.

@spatula75 yes, the sequence is correct. It is sort of the same today but recovery is kicked off by one of the shutdown listeners so it's not clear whether a connection might already be in recovery by the time a user-provided listener is executed.