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

Recovery attempts limit is never reset #408

Closed
camelpunch opened this Issue May 31, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@camelpunch
Contributor

camelpunch commented May 31, 2016

Something interesting seems to be happening with recovery. There's a line that decrements attempts but no accompanying reset. However, channel operations seem to recover on their own even when the number of attempts is set low.

Not a huge deal, but thought I'd record this thought before forgetting it.

@camelpunch camelpunch changed the title from Recovery attempts aren't reset to Recovery attempts limit is effectively ignored Jun 13, 2016

camelpunch added a commit to camelpunch/bunny that referenced this issue Jun 13, 2016

Rework recovery test
- Wait for state changes on HTTP API, with timeouts. This is about 3x
  faster than the previous constant sleeps on my machine.
- Silence log output
- Pend out an example that was incorrectly passing
  (attempt limits: ruby-amqp#408)

Issue #410

michaelklishin added a commit that referenced this issue Jun 22, 2016

Rework recovery test
- Wait for state changes on HTTP API, with timeouts. This is about 3x
  faster than the previous constant sleeps on my machine.
- Silence log output
- Pend out an example that was incorrectly passing
  (attempt limits: #408)

Issue #410
@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Feb 24, 2017

Member

I did some poking around and think that the author (seems to be @jafrog in fff7f44) intended it to work like this: the counter is global for the entire lifetime of a connection object.

I agree that this may be counterintuitive to most and decided to change it. One issue with
the per-recovery attempt limit is that Bunny tests that use HTTP client to force close connections
cannot simulate counted retries (TCP failures) easily since the server is up and thus any reconnection will succeed and reset the counter. I don't have a solution that's not environment-specific or fragile (e.g. stopping RabbitMQ and trying to start ti back), so leaning towards deleting the test after a lot of manual testing _(ツ)_/¯.

Member

michaelklishin commented Feb 24, 2017

I did some poking around and think that the author (seems to be @jafrog in fff7f44) intended it to work like this: the counter is global for the entire lifetime of a connection object.

I agree that this may be counterintuitive to most and decided to change it. One issue with
the per-recovery attempt limit is that Bunny tests that use HTTP client to force close connections
cannot simulate counted retries (TCP failures) easily since the server is up and thus any reconnection will succeed and reset the counter. I don't have a solution that's not environment-specific or fragile (e.g. stopping RabbitMQ and trying to start ti back), so leaning towards deleting the test after a lot of manual testing _(ツ)_/¯.

michaelklishin added a commit that referenced this issue Feb 24, 2017

Change recovery attempt limit behavior to match #408
Previous behehavior is not unreasonable but is not what
many users and even RabbitMQ team members come to expect.
Therefore it can be treated as a bug.

Previously a reconnection counter was preserved between
successful recoveries. This made the integration test
that uses server-sent connection.close possible.

With this change, the counter is reset after successful
reconnection but there's an option to go back to the original
behavior. We also do a hell of a lot more logging.
Note that connection.close recovery with a running node will
no longer work for testing this functionality. There aren't
may ways to stop and start the node that aren't
environment-specific, perhaps one day we will mess
with nodes in Bunny tests and introduce a replacement test.

References fff7f44.

@michaelklishin michaelklishin self-assigned this Feb 24, 2017

@michaelklishin michaelklishin changed the title from Recovery attempts limit is effectively ignored to Recovery attempts limit is never reset Feb 24, 2017

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