search

eandersson / amqpstorm

Thread-safe Python RabbitMQ Client & Management library
https://www.amqpstorm.io/
MIT License
186 stars 36 forks source link

Handle all channel opening within the lock #99

Closed eandersson closed 3 years ago

eandersson commented 3 years ago

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.

eandersson commented 3 years ago

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

try:
<lock>
<do things>
<return>
finally:
<release lock>
<log message>
eandersson commented 3 years ago

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 commented 3 years ago

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 https://github.com/eandersson/amqpstorm/pull/99#issuecomment-847497730, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABJGJXEKDMD3LCGYRUR7WDTPMJHBANCNFSM45MOAL3A .

eandersson commented 3 years ago

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