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

Reset channel unconfirmed_set on network recovery #406

Merged
merged 2 commits into from May 20, 2016

Conversation

Projects
None yet
2 participants
@biinari
Contributor

biinari commented May 19, 2016

With publisher confirms and automatic recovery enabled, when a network failure occurs, acks / nacks no longer get pushed onto the confirms continuations queue. This causes future wait_for_confirms to fail, even though the messages published since network recovery have been acknowledged by the broker.

I have included a spec with this PR that catches the problem and a fix that resets unconfirmed_set on network recovery. I tried to keep the stress spec as concise as possible but it does take around 20-30 seconds to run.

Any suggestions to improve this are welcome. In particular I wasn't sure whether to put my spec into spec/issues instead.

biinari added some commits May 19, 2016

Stress spec: ensure all messages consumed
wait_for_confirms for each batch of messages and ensure they have all been
confirmed.

Ensure all ids get received by subscriber at least once
Reset unconfirmed_set on network recovery
Unconfirmed messages will never be confirmed by broker after network
connection loss. Reset unconfirmed_set on recovery

@michaelklishin michaelklishin self-assigned this May 20, 2016

@michaelklishin michaelklishin merged commit e14cf84 into ruby-amqp:master May 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin May 20, 2016

Member

Makes sense. Thank you.

Member

michaelklishin commented May 20, 2016

Makes sense. Thank you.

@biinari

This comment has been minimized.

Show comment
Hide comment
@biinari

biinari May 20, 2016

Contributor

Thanks @michaelklishin for the quick merge.

Contributor

biinari commented May 20, 2016

Thanks @michaelklishin for the quick merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment