Skip to content

[Messenger][AMQP] Automatically reconnect on connection loss #53892

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

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

ostrolucky
Copy link
Contributor

@ostrolucky ostrolucky commented Feb 10, 2024

Q A
Branch? 7.1
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

When using Rabbitmq in cluster, there is a common need of having to upgrade the nodes, while keeping the existing connections. The way this is normally done is by putting nodes in cluster to maintenance mode, ensuring cluster is healthy at all times. However, symfony/messenger nor php-amqp handle this use case at the moment. What happens instead is that exception when getting the message is thrown, worker crashes, error is logged and process manager has to respin it. This all happens without having a way in user space to handle this case better. Messenger's retry mechanism does not work here, because that one kicks in only when exception is thrown in handlers. Concrete exception is following:

[AMQPConnectionException (320)]
Server connection error: 320, message: CONNECTION_FORCED - Node was put into maintenance mode

What I'm proposing in this PR is that if connection error is detected try to reconnect once before throwing exception. That should handle the outlined case. This goes line in line with recommendation from AWS's support we got:

kindly ensure that the client connecting to the broker attempts a retry in case the above error message is observed during a maintenance window. In RabbitMQ cluster deployments, the nodes are restarted one-by-one, meaning at least two nodes will be up and running at all times. Even if a connection is severed, a connection retry will result in the other nodes accepting the connection, and the clients can keep using the broker.

I've also reported issue at php-amqplib/php-amqplib#1161 with hope that this could be fixed at some point upstream, but I don't give it a big chance.

@OskarStark OskarStark changed the title [Messenger] AMQP:Automatically reconnect on connection loss [Messenger][AMQP] Automatically reconnect on connection loss Feb 23, 2024
@ostrolucky ostrolucky force-pushed the amqp-connection-reconnect branch from d5b946d to ae95410 Compare February 23, 2024 09:41
@ostrolucky ostrolucky force-pushed the amqp-connection-reconnect branch from ae95410 to 056b4a5 Compare February 23, 2024 10:10
@ostrolucky
Copy link
Contributor Author

ci failures unrelated

@fabpot
Copy link
Member

fabpot commented Feb 23, 2024

Thank you @ostrolucky.

@fabpot fabpot merged commit f78f932 into symfony:7.1 Feb 23, 2024
@fabpot fabpot mentioned this pull request May 2, 2024
nicolas-grekas added a commit that referenced this pull request Jan 17, 2025
…nPillevesse)

This PR was merged into the 7.1 branch.

Discussion
----------

[Messenger] [AMQP] Improve AMQP connection issues

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| License       | MIT

Appy this fix #53892 for :
- `ack` (https://github.com/php-amqp/php-amqp/blob/latest/stubs/AMQPQueue.php#L50)
- `nack` (https://github.com/php-amqp/php-amqp/blob/latest/stubs/AMQPQueue.php#L238)

Because `\AMQPConnectionException` can also happen here

Commits
-------

5356023 improve amqp connection issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants