What library version is used?

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

AutorecoveringConnection#BeginAutomaticRecovery does check ManuallyClosed.

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.

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

@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?

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.

@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.

@LoungeFlyZ we can make it idempotent for 5.0.0.

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?

@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.

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

I think it is ok for Close to throw. There may be rare but valid failures a user would like to handle. AlreadyClosedException I don't think anyway will miss. Abort should be the non throwing option as it currently is.

Whilst we're at it: Dispose calls Abort but then throws some exception anyway if there was an error during Abort. I'd like to change this to make Dispose just call Abort and ignore errors which should be more intuitive to users which can catch errors during Close should they want to.

@kjnilsson sounds good.

OK I've updated the PR not to throw exceptions in Dispose. Now we've clarified the semantics of these methods it should be easier to detect transgressions.