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

client: pass the provided ctx to backoff.RetryNotify #146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wwade
Copy link

@wwade wwade commented Jan 24, 2023

If we hit a connection error in any of the ..WithContext functions, we end up blocked in the backoff.RetryNotify loop. This loop can use a context, but it needs to be bound to the BackOff provided when RetryNotify is called.

Added a test case TestSubscribeWithContextAbortRetrier to demonstrate the problem and verify the fix.

Fixes #131.

If we hit a connection error in any of the ..WithContext functions, we
end up blocked in the backoff.RetryNotify loop. This loop can use a
context, but it needs to be bound to the BackOff provided when
RetryNotify is called.

Added a test case `TestSubscribeWithContextAbortRetrier` to
demonstrate the problem and verify the fix.

Fixes r3labs#131.
@wwade
Copy link
Author

wwade commented Feb 1, 2023

@purehyperbole mind having a look at this one?

@wwade
Copy link
Author

wwade commented Feb 27, 2023

@merlinran mind having a look?

Copy link
Contributor

@merlinran merlinran 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 like a neat solution which solves the problem it intended to solve. I'm not part of the org so still need the members to decide if merge or not.

@wwade
Copy link
Author

wwade commented Feb 27, 2023

I'm not part of the org so still need the members to decide if merge or not.

Ah, gotcha, well, thanks for the review just the same!

@wwade
Copy link
Author

wwade commented Feb 27, 2023

bump @purehyperbole 😄

@wwade
Copy link
Author

wwade commented Apr 6, 2023

@purehyperbole would appreciate your review to get this one merged if it looks ok.

@wwade
Copy link
Author

wwade commented May 5, 2023

@purehyperbole could you please have a look at merging this?

@mgabeler-lee-6rs
Copy link

@purehyperbole Is this package still maintained?

@reidjc As the only other publicly visible member of the r3labs organization, can you comment on the status of this repo?

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.

Context cancelled should be respected
4 participants