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

increase reliability when dealing with closed channels and closure notifications #34

Merged
merged 1 commit into from
Aug 29, 2018

Conversation

puellanivis
Copy link
Collaborator

If the emit channel is closed, the code will never reopen it. So, we should return an error in this case, as the rabbus.Rabbus has entered into a fail state, and cannot continue.

NotifyClose stacks channels into a slice https://github.com/streadway/amqp/blob/master/channel.go#L444 This means that calling this multiple times results in a long slice of notification channels all of which have nobody listening on them because the iteration of the switch they are from is now gone. When a close event occurred, it would send an error on all of the channels, so that the last NotifyClose should still get its error message, but since they were generally unbuffered, it’s just as likely that the close notification process would block indefinitely on the channel send, waiting on a receiver that no longer is possible.

Finally, when NotifyClose triggered, and re-establishing a consumer failed, we were closing all of the channels. (This enters the fail state noted above where the emit channel is now closed.) But by setting the r.reconn to a length of 10, some of the reconnect values kept being delivered while waiting for the end of the channel, allowing other reconnections to build back up, but this would hide the problem that we were actually closing the whole rabbus.Rabbus, which is not what the code clearly intended to do.

@rafaeljesus
Copy link
Owner

@puellanivis thanks for very detailed PR description, actually this issue was spotted here #31 altho I completely forgot to merge that. Thank you

@rafaeljesus rafaeljesus merged commit f0fbeaa into rafaeljesus:master Aug 29, 2018
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