Closing an AutoRecoveringConnection does not stop it from continuing recovery forever

[LoungeFlyZ]

Once an AutoRecoveringConnection connection enters auto recovery and trying to recover the connection will continue to do try forever even if the connection is manually closed.

I am not super familiar with the code, but believe the issue lays in the RecoverConnectionDelegate method that enters a while loop and fails to check if the connection has been manually closed via the manuallyClosed variable.

The order for a repro is:

1. Create and Open auto recovering connection
2. Sever the network connection between the client and rabbit server. The connection will start attempting auto-recovery.
3. Call .Close() and/or .Dispose() on the connection
4. Observe that the connection continues to try and auto-recover ... forever.

I would have expected that the connection would have given up trying to reconnect after it was closed, but i might be missing some subtle issue here that i am not aware of.

[michaelklishin]

What library version is used?

See #290 and #291. Dispose is NOT how connections are closed.

[michaelklishin]

AutorecoveringConnection#BeginAutomaticRecovery does check ManuallyClosed.

[kjnilsson]

Yes this sounds fair to me. ManuallyClosed should be checked before each reconnection attempt. Arguably it doesn't need to be checked where it is atm at all.

[kjnilsson]

Raised a PR for this. It does change the semantics of Close and Abort so there may be something to discuss there.

[michaelklishin]

@kjnilsson we can throw unless there's recovery in progress. This behaviour (Close throws an AlreadyClosedException when invoked more than once or on an otherwise closed connection) has been around forever. I'm not sure if it's worth keeping, an idempotent Close/Abort seem more user-friendly to me.

@LoungeFlyZ @bording thoughts?

[kjnilsson]

It probably makes sense to move the IsOpen checks into the Connection instead then non-recovering connections would also behave the same. Abort appears to be meant to already have this behaviour but I'm not sure the current implementation work well with the auto recovery logic in this regard.

[LoungeFlyZ]

@michaelklishin @kjnilsson I think most would expect Abort/Close to be idempotent also so i think that would be a good change. Right now in a few places when I Close/Abort I just try/catch around them but do nothing with the exceptions due to this.

I'll take a look at the PR, but heading away on vacation for a few days, so replies might be slow.

Thanks a lot for looking into this. I actually stumbled across this due to an odd situation where we lose our connection for a while and during that time the password is reset for the login, we negotiate with our API for the new credentials and tried to close/shutdown the old connection so that we can create a new one with the new creds. We noticed however that the reconnecting thread was still attempting to reconnect the old connection. I actually think this could lead to memory leaks as that old connection object sticks around and it has a reference to the connection factory also ... which i don't think will ever be let go due to this thread never ending. Our workaround for this currently, is to update the old connection factory user credentials and the old connection will then reconnect. But we would VERY much like to just close the old connection and spin up a new one ... much cleaner.

[michaelklishin]

@LoungeFlyZ we can make it idempotent for 5.0.0.

[bording]

I'm not sure if it's worth keeping, an idempotent Close/Abort seem more user-friendly to me.

This does seem like a good change to make, and since you're already planning the 5.0.0 release anyway, any concerns about a behavior change are mitigated as well.

If you are going to make Close no longer throw, that does bring into question, what is the point of keeping both Close and Abort around at that point since Abort is a non-throwing Close?

[michaelklishin]

@bording Abort is supposed to ignore possible I/O-related and other rare but possible exceptions. I don't think that aspect changes. Also, removing Abort entirely is going to break way more existing code than the idempotent Close. Thank you for chiming in, by the way.

@LoungeFlyZ I can't promise when a 5.0 may drop since @kjnilsson and I won't have a chance to actively work on this in the upcoming months. But we will merge this one way or another this or next week and will release a milestone, so there will be at least something you can depend on.

[LoungeFlyZ]

@michaelklishin thanks. That would be great. Much appreciated.