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

issue656 - change recoverable_network_failure? #658

Merged
merged 2 commits into from
Feb 23, 2023
Merged

issue656 - change recoverable_network_failure? #658

merged 2 commits into from
Feb 23, 2023

Conversation

womblep
Copy link
Contributor

@womblep womblep commented Feb 21, 2023

Alter recoverable_network_failure? to check against a definable array of exceptions and modify recover_from_network_failure to handle the exception logic differently

…y of exceptions and modify recover_from_network_failure to handle the exception logic differently
Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

This looks good but I get two spec failures on Ruby 3.2 (macOS on ARM64):

Failures:

  1) Bunny::Session when retry attempts have been exhausted closes the session
     Failure/Error: expect(session.closed?).to be true

       expected true
            got false
     # ./spec/issues/issue549_spec.rb:20:in `block (3 levels) in <top (required)>'

  2) Bunny::Session when retry attempts have been exhausted stops the reader loop
     Failure/Error: expect(reader_loop.stopping?).to be true

       expected true
            got false
     # ./spec/issues/issue549_spec.rb:27:in `block (3 levels) in <top (required)>'

Finished in 42.48 seconds (files took 0.2137 seconds to load)
26 examples, 2 failures

Failed examples:

rspec ./spec/issues/issue549_spec.rb:17 # Bunny::Session when retry attempts have been exhausted closes the session
rspec ./spec/issues/issue549_spec.rb:23 # Bunny::Session when retry attempts have been exhausted stops the reader loop

@michaelklishin
Copy link
Member

This is a test inter-dependency. When I run just one spec file, it passes 100 times out of 100.

When I run

rspec spec/issues/

then I get these two failures.

@womblep
Copy link
Contributor Author

womblep commented Feb 22, 2023

This is a test case for a different issue. Does it happen before the change too?
And for me running all the specs in issues or running it individually causes the same errors

@womblep
Copy link
Contributor Author

womblep commented Feb 22, 2023

I picked out all the errors that I could find that were defined and added them to Session->DEFAULT_RECOVERABLE_EXCEPTIONS
StandardError was not one of them, should it be?
If I change the test to:
session = create_session
session.recoverable_exceptions << StandardError
session.handle_network_failure(StandardError.new)
then it passes, suggesting that StandardError should be trapped

@michaelklishin
Copy link
Member

It's a good question since StandardError is almost a catch-all in Ruby. But we can try this.

@michaelklishin michaelklishin merged commit 09c1bf7 into ruby-amqp:main Feb 23, 2023
@michaelklishin michaelklishin added this to the 2.21.0 milestone Feb 23, 2023
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