transport: fix logical race in flow control #1005
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
rhysh
force-pushed
the
issue_632_test
branch
from
e1dc5ab
to
4217c52
Compare
iamqizhao
reviewed
Nov 30, 2016
| @@ -614,9 +614,6 @@ func (t *http2Client) Write(s *Stream, data []byte, opts *Options) error { | |||
| // Wait until the transport has some quota to send the data. | |||
| tq, err := wait(s.ctx, s.done, s.goAway, t.shutdownChan, t.sendQuotaPool.acquire()) | |||
| if err != nil { | |||
| if _, ok := err.(StreamError); ok || err == io.EOF { | |||
| t.sendQuotaPool.cancel() | |||
| } | |||
There was a problem hiding this comment.
With this PR, I think we should be able to remove the "add" calls at line#607 and line#613. This applies to the server side too.
| @@ -3188,6 +3188,124 @@ func TestServerCredsDispatch(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestFlowControlIssue632(t *testing.T) { | |||
There was a problem hiding this comment.
this is not a good name. how about "TestFlowControlRace"?
There was a problem hiding this comment.
Done. "TestFlowControlLogicalRace", to emphasize that is wouldn't be caught with Go's (data) race detector.
| go s.Serve(lis) | ||
|
|
||
| ctx := context.Background() | ||
| ctx, _ = context.WithTimeout(ctx, totalTimeout) |
There was a problem hiding this comment.
go test has timeout mechanism. You do not need to have your own here.
| } | ||
| }() | ||
|
|
||
| cc, err := grpc.Dial(lis.Addr().String(), grpc.WithInsecure()) |
There was a problem hiding this comment.
add grpc.WithBlock() to make the procedure more clear.
iamqizhao
reviewed
Dec 1, 2016
| } | ||
| } | ||
|
|
||
| type issue632server struct { |
Remove the add and cancel methods of quotaPool. Their use is not required, and leads to logical races when used concurrently from multiple goroutines. Rename the reset method to add. The typical way that a goroutine claims quota is to call the add method and then to select on the channel returned by the acquire method. If two goroutines are both trying to claim quota from a single quotaPool, the second call to the add method can happen before the first attempt to read from the channel. When that happens the second goroutine to attempt the channel read will end up waiting for a very long time, in spite of its efforts to prepare the channel for reading. The quotaPool will always behave correctly when any positive quota is on the channel rather than stored in the struct field. In the opposite case, when positive quota is only in the struct field and not on the channel, users of the quotaPool can fail to access available quota. Err on the side of storing any positive quota in the channel. This includes a reproducer for grpc#632, which fails on many runs with this package at v1.0.4. The frequency of the test failures depends on how stressed the server is, since it's now effectively checking for weird interleavings of goroutines. It passes reliably with these changes to the transport package. The behavior described in grpc#734 (an RPC with a streaming response hangs unexpectedly) matches what I've seen in my programs, and what I see in the test case added here. If it's a logical flow control bug, this change may well fix it. Updates grpc#632 Updates grpc#734
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.
Remove the add and cancel methods of quotaPool. Their use is not required, and
leads to logical races when used concurrently from multiple goroutines. Rename
the reset method to add.
The typical way that a goroutine claims quota is to call the add method and
then to select on the channel returned by the acquire method. If two
goroutines are both trying to claim quota from a single quotaPool, the second
call to the add method can happen before the first attempt to read from the
channel. When that happens the second goroutine to attempt the channel read
will end up waiting for a very long time, in spite of its efforts to prepare
the channel for reading.
The quotaPool will always behave correctly when any positive quota is on the
channel rather than stored in the struct field. In the opposite case, when
positive quota is only in the struct field and not on the channel, users of
the quotaPool can fail to access available quota. Err on the side of storing
any positive quota in the channel.
This includes a reproducer for #632, which fails on many runs with this
package at v1.0.4. The frequency of the test failures depends on how stressed
the server is, since it's now effectively checking for weird interleavings of
goroutines. It passes reliably with these changes to the transport package.
The behavior described in #734 (an RPC with a streaming response hangs
unexpectedly) matches what I've seen in my programs, and what I see in the
test case added here. If it's a logical flow control bug, this change may well
fix it.
Updates #632
Updates #734