Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection: forcibly close stream if close() is called while the connection is blocked #744

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SimonWoolf
Copy link

Because of how rabbit's connection.blocked mode works, if the connection is blocked, the client will not receive a close-ok unless/until the connection becomes unblocked, which may be some time away, or never. Current behaviour just leaves the connection open indefinitely. This instead closes it immediately.

if the connection is blocked, we will not receive a close-ok until the
connection becomes unblocked, which may be some time. So just terminate
the connection immediately.
@cressie176
Copy link
Collaborator

Hi @SimonWoolf, Thanks for the PR. I see other clients have suffer from the same challenge.

@michaelklishin, it was a while back but do you remember whether and how this issue was address with the Java client?
https://groups.google.com/g/rabbitmq-users/c/7cQXglAnWS8

Is there a defacto standard approach that amqplib should follow?

@cressie176
Copy link
Collaborator

I've been thinking about this some more and wondering whether supplying an optional boolean "force" to the connection close method might be better. @SimonWoolf if you have time to update the PR then I'll merge. Otherwise I'll take a look, but probably not for a little while.

@SimonWoolf
Copy link
Author

I've been thinking about this some more and wondering whether supplying an optional boolean "force" to the connection close method might be better.

to be clear, are you asking for that force arg to apply to the 'destroy() after end()' behaviour change, or to the 'close immediately if blocked' change? (or both?)

@cressie176
Copy link
Collaborator

I was thinking of adding the option to connection.close, then passing this through. The function also takes an optional callback which would make this a bit clumsy. Also, something I hadn't considered before, is that this wouldn't help for any non-user invoked closes.

@cressie176
Copy link
Collaborator

@cressie176
Copy link
Collaborator

php-amqplib took a timeout approach - php-amqplib/php-amqplib#609

@kibertoad
Copy link
Collaborator

@SimonWoolf Ping? Are you still open to update the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants