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

Don't try to close a connection that is in an error state #400

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

desiderius
Copy link
Contributor

If a client try to gracefully shutdown when it encounter an error it can, potentially, wait forever for a reply to the connection::close() method. We see that a lot in our test environment where we kill services in arbitrary order to see if they can recover automatically.

So this commit try to mitigate that.

If a connection is in an error state (a restart of the RabbitMQ server
for example), we shouldn't try to ask the server to close it,
otherwise we potentially have to wait forever for the close-ok
response.
@slavik-pastushenko
Copy link
Contributor

have absolutely the same issue, can we merge it, please? cc @Keruspe

@Keruspe
Copy link
Collaborator

Keruspe commented Apr 21, 2024

The fix looks good but incomplete. Instead of checking whether we're in error state, we should check if we're connected. This way there won't be problems with connections that are closing either

@Keruspe
Copy link
Collaborator

Keruspe commented Apr 21, 2024

We probably need the same for Channel I guess?

If a connection is not in the connected state (e.g. an error state due
to a restart of the RabbitMQ server), we shouldn't try to ask the
server to close it, otherwise we might have to wait forever for the
close-ok response.
@desiderius
Copy link
Contributor Author

The fix looks good but incomplete. Instead of checking whether we're in error state, we should check if we're connected.

Done.

We probably need the same for Channel I guess?

The Channel::do_channel_close method already check for the connected status, so it's good there.

@Keruspe Keruspe merged commit b49d907 into amqp-rs:main Apr 22, 2024
8 checks passed
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