-
Notifications
You must be signed in to change notification settings - Fork 211
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
[Merged by Bors] - p2p: server: close streams upon context cancellation #5981
Conversation
Also, cancel the context provided to the server handlers when the handler finishes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5981 +/- ##
=========================================
+ Coverage 80.7% 81.1% +0.3%
=========================================
Files 288 292 +4
Lines 29894 30860 +966
=========================================
+ Hits 24149 25043 +894
- Misses 4158 4182 +24
- Partials 1587 1635 +48 ☔ View full report in Codecov by Sentry. |
p2p/server/server.go
Outdated
ctx, cancel := context.WithCancel(ctx) | ||
eg.Go(func() error { | ||
<-ctx.Done() | ||
// deadlineAdjuster.Close() is safe to call multiple times | ||
req.stream.Close() | ||
return nil | ||
}) | ||
eg.Go(func() error { | ||
defer cancel() |
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.
I think this does the same without needing to spawn a dedicated go routine?
ctx, cancel := context.WithCancel(ctx) | |
eg.Go(func() error { | |
<-ctx.Done() | |
// deadlineAdjuster.Close() is safe to call multiple times | |
req.stream.Close() | |
return nil | |
}) | |
eg.Go(func() error { | |
defer cancel() | |
eg.Go(func() error { | |
defer req.stream.Close() |
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.
No, there's a critical difference: if the context is canceled while the handler is still executing (e.g. the server is stopped in a test by means of canceling the context passed to its Run
method), a blocking I/O operation may still hang and the handler will never exit, also this defer
will never be executed. In my implementation, the stream is closed immediately after the context is canceled, which may happen not only due defer cancel()
, but also b/c the context passed to Run
is canceled; closing the stream interrupts any current blocking operations
bors merge |
## Motivation When a P2P Server request is canceled or a server is stopped while serving a request, some code paths that involve blocking I/O calls may hang b/c the P2P stream is not closed upon cancellation of the context that was passed to `StreamRequest` / `Request` or `Run`. On top of that, the context which is passed to server handlers is not canceled when the server exits. This was split off #5769 where a P2P test was hanging when trying to stop the P2P servers.
Pull request successfully merged into develop. Build succeeded: |
Motivation
When a P2P Server request is canceled or a server is stopped while serving a request, some code paths that involve blocking I/O calls may hang b/c the P2P stream is not closed upon cancellation of the context that was passed to
StreamRequest
/Request
orRun
. On top of that, the context which is passed to server handlers is not canceled when the server exits.This was split off #5769 where a P2P test was hanging when trying to stop the P2P servers.
Description
Close P2P server streams upon context cancellation. Cancel handlers' context when the handler functions exit