Skip to content

fix(mqs): redial dropped RabbitMQ connections instead of wedging until restart#999

Open
LendritIbrahimi wants to merge 1 commit into
hookdeck:mainfrom
LendritIbrahimi:lendritibrahimi/fix/rabbitmq-reconnect
Open

fix(mqs): redial dropped RabbitMQ connections instead of wedging until restart#999
LendritIbrahimi wants to merge 1 commit into
hookdeck:mainfrom
LendritIbrahimi:lendritibrahimi/fix/rabbitmq-reconnect

Conversation

@LendritIbrahimi

Copy link
Copy Markdown

This fixes the issue where a dropped RabbitMQ connection permanently wedged the queue: the connection was dialed once via sync.Once, so after a broker restart or network blip every subsequent publish failed against the closed connection until the whole pod was restarted. (explained in this issue #998)

The implementation replaces the sync.Once with a mutex-guarded ensureConnected that lazily redials whenever the current connection is nil or closed, reopening the topic on the new connection (the stale topic is shut down in the background). Publish additionally retries once: if a publish fails and the connection turns out to be lost, it redials and re-attempts before surfacing the error.

If the redial itself fails, the original publish error is returned rather than the dial error, so callers see the real failure. Subscribe goes through the same path, so consumers created after a drop also get a fresh connection.

@alexluong alexluong left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM and thanks for raising the issue!

Got a few comments here, would appreciate it you can address or respond here

1: I think there's some formatter issue, can you make sure to run gofmt on the codebase?
2: The redial mechanism is fair but I wonder if there's anything we should consider in case of an outage since every Publish will attempt a redial which may or may not be the desired behavior. I'm okay moving forward with this behavior as is, just want to flag this in case you want to rethink this mechanism.

Overall, great job and appreciate your contribution!

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.

2 participants