-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix: context cancellation handling #22824
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
fix: context cancellation handling #22824
Conversation
Signed-off-by: sivchari <[email protected]>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22824 +/- ##
==========================================
+ Coverage 56.06% 56.11% +0.04%
==========================================
Files 343 343
Lines 57545 57545
==========================================
+ Hits 32262 32289 +27
+ Misses 22633 22612 -21
+ Partials 2650 2644 -6 ☔ View full report in Codecov by Sentry. |
|
@sivchari is this something we could test? Obviously we could unit test the function, but something more end-to-end would be nice if possible. |
|
@crenshaw-dev The following 2 test has already tested this function on e2e, then these are sometime stuck since the deadline doesn't be handled. I tried to reproduce on my machine, but it's difficult to reproduce since this failure is important for timing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! @crenshaw-dev are we good to merge this?
Signed-off-by: sivchari <[email protected]> Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: sivchari <[email protected]> Signed-off-by: dsuhinin <[email protected]>
Signed-off-by: sivchari <[email protected]> Signed-off-by: enneitex <[email protected]>
Checklist:
Currently, isCanceledContextErr can only handle the context.Canceled and gRPC status when the communication is hung up. So if first communication is succeeded, then second communication passed context that has already been deadline exceeded, the loop is no longer break since the context.DeadlineExceeded isn't considered. So I made it consider the context.DeadlineExceeded.