fix(RateLimitRoundtripper): Fix mutex leak and not respecting context cancellation#2298
Conversation
636a4db to
2795869
Compare
2795869 to
217adae
Compare
217adae to
a6a9f0d
Compare
|
Hey @pete-woods 👋 Thanks for the contribution! Would you be available to resolve the merge conflict? Would it also be possible to explain in more detail how the changes work? |
|
Absolutely - I'll rebase when I get a moment |
a6a9f0d to
ce27887
Compare
|
Okay, rebased and resolved the conflict now |
ce27887 to
40a8d48
Compare
|
@nickfloyd sorry for direct @ - is there any chance you have time to look at this PR that tries to fix a deadlock/cancellation issue? (You're the #1 contributor to the repo) |
|
@pete-woods It's Thanksgiving, so most US-based folks won't be around. We've raised this PR as something to look at next week :) |
d20eee6 to
4cb9825
Compare
|
@pete-woods could you please rebase and I'll see if we can get it merged. |
4cb9825 to
d9c33b1
Compare
Done |
d9c33b1 to
0d7ec81
Compare
…ellation - Make sure that all exit paths from functions correctly release the mutex. - Add a sleep function that respects context cancellation, and use it in-place of time.Sleep.
0d7ec81 to
d612fae
Compare
stevehipwell
left a comment
There was a problem hiding this comment.
Thanks for this PR @pete-woods.
FYI I'm not sure this is going to have much of an immediate impact given the general lack usage of the context based provider methods (most call to the client use context.Background()). That said it will provide a noticeable benefit to resources where we adopt the context based patterns.
LGTM
It's been quite some time since I made this PR, but some brain cells are telling me it was getting stuck in the sleep between retries, at least as best as I can remember. |
|
@pete-woods yes it does, but I suspect that it will still do so in most cases as there is no context in the legacy methods to propagate down. Your fix looks a lot like the pattern in the library I use when writing GitHub apps, which coincidentally is where I'd like to move this implementation. I think we'll also need to look at the default context timeout and add provider scoped timeout configuration for retries as the default is 20 mins. |
Resolves #2299
Before the change?
After the change?
Pull request checklist
[ ] Docs have been reviewed and added / updated if needed (for bug fixes / features)Does this introduce a breaking change?
Please see our docs on breaking changes to help!