stream: fix calloption.After() race in finish #3672
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dfawley
reviewed
Jun 9, 2020
test/end2end_test.go
Outdated
Comment on lines
7150
to
7186
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| var p peer.Peer | ||
| stream, err := ss.client.FullDuplexCall(ctx, grpc.Peer(&p)) | ||
| if err != nil { | ||
| t.Fatalf("_.FullDuplexCall(_) = _, %v", err) | ||
| } | ||
| if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { | ||
| t.Fatalf("_ has error %v while sending", err) | ||
| } | ||
| if _, err := stream.Recv(); err != nil { | ||
| t.Fatalf("%v.Recv() = %v", stream, err) | ||
| } | ||
| cancel() | ||
| if _, err := stream.Recv(); status.Code(err) != codes.Canceled { | ||
| t.Fatalf("%v compleled with error %v, want %s", stream, err, codes.Canceled) | ||
| } | ||
| // If recv returns before call options are executed, peer.Addr is not set, | ||
| // fail the test. | ||
| if p.Addr == nil { | ||
| t.Fatalf("peer.Addr is nil, want non-nil") | ||
| } |
There was a problem hiding this comment.
Do we need to do this in a loop to have a better chance of stimulating the race?
dfawley
reviewed
Jun 10, 2020
rpc_util.go
Outdated
| if c.stream != nil { | ||
| *o.HeaderAddr, _ = c.stream.Header() | ||
| func (o HeaderCallOption) after(c *callInfo, attempt *csAttempt) { | ||
| if attempt != nil { |
There was a problem hiding this comment.
Can we now assume this is never called with a nil attempt or attempt.s?
There was a problem hiding this comment.
If rpc is canceled before we make the attempt (e.g. blocking on pick)?
There was a problem hiding this comment.
But there's only one call site (right?) and it's guarded by if attempt != nil && attempt.s != nil.
There was a problem hiding this comment.
Yeah, right. Removed
dfawley
approved these changes
Jun 10, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Race:
After cancel, two calls to stream.Finish()
grpc-go/stream.go
Line 790 in 479df5e
grpc-go/stream.go
Line 1089 in 479df5e
grpc-go/stream.go
Line 745 in 479df5e
Call 1 happens first, sets
finished=true, before callingafter()Call 2 happens now, return early because
finished==true. So RecvMsg() also returnsUser sees error from RecvMsg(), but because 1 still hasn't call
after(), peer is not set.This fix:
after() should work on attempt instead, because after() is only called in finish,
and the stream should have been committed.