Handle all channel opening within the lock

[eandersson]

Moved both the _cleanup_channel and return under the thread lock. We can't have the channel being cleaned up before the function return, as it will result in a KeyError, but we need to return a channel to not break the context manager.

[jhogg]

Looks good for the current conditions. My only concern, which we can roll to the 3.x work, is the LOGGER call is not deterministic and I don't like them inside lock()s unless absolutely necessary.

[eandersson]

Yea - the logger has bitten me before. I could just change it to

try:
<lock>
<do things>
<return>
finally:
<release lock>
<log message>

[eandersson]

An exception thrown in channel.open() will still result in a successful log message.

(Side question: Do you have a chat/discussion channel for amqpstorm other than issue tracking?)

Yea - not a lot we can do about that unfortunately without some larger changes. There are other log events as well, but I guess that most of those are on a separate thread.

Nvm. I see what you meant. Good catch.

I set a slack workspace up
https://join.slack.com/t/eandersson/shared_invite/zt-qy4nqk3l-A3ZYzrUPmkIoI4xRDLMojA

[jhogg]

Erik, Commit it. Call it done for now. I agree this is one of those situations where the effort to make the logging "perfect" for a corner case is going to far outweigh the benefit. Likewise, I have been bit by logging causing a race condition due to a blocking call/task switch. My direct email - ***@***.*** (personal, I'll see it) or ***@***.*** (it risks getting lost) Jay

…

On Mon, May 24, 2021 at 10:18 PM Erik Olof Gunnar Andersson < ***@***.***> wrote: An exception thrown in channel.open() will still result in a successful log message. (Side question: Do you have a chat/discussion channel for amqpstorm other than issue tracking?) Yea - not a lot we can do about that unfortunately without some larger changes. There are ton of debug messages etc within the channel.open. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABJGJXEKDMD3LCGYRUR7WDTPMJHBANCNFSM45MOAL3A> .

[eandersson]

Sounds good. The email isn't showing up, but feel free to join the Slack workspace I set up.
https://join.slack.com/t/amqpstorm/shared_invite/zt-qy4nqk3l-A3ZYzrUPmkIoI4xRDLMojA