Skip to content
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

transport: call Unlock in defer to avoid data race #2953

Merged
merged 1 commit into from
Aug 7, 2019
Merged

transport: call Unlock in defer to avoid data race #2953

merged 1 commit into from
Aug 7, 2019

Conversation

lzhfromustc
Copy link
Contributor

Description
Among 8 read/write operations to f.delta, 7 of them are protected by f.mu.Lock(), but the one at line 173 of file grpc/internal/transport/flowcontrol.go is not protected. In order to avoid potential data race here, I use defer f.mu.Unlock() to make sure that all usages of f.delta is in critical section.

Buggy code
The last read operation of f.delta happens after f.mu.Unlock():

if estUntransmittedData > estSenderQuota {
// Sender's window shouldn't go more than 2^31 - 1 as specified in the HTTP spec.
if f.limit+n > maxWindowSize {
f.delta = maxWindowSize - f.limit
} else {
// Send a window update for the whole message and not just the difference between
// estUntransmittedData and estSenderQuota. This will be helpful in case the message
// is padded; We will fallback on the current available window(at least a 1/4th of the limit).
f.delta = n
}
f.mu.Unlock()
return f.delta
}

@easwars easwars self-assigned this Aug 7, 2019
@easwars easwars self-requested a review August 7, 2019 19:56
@easwars easwars removed their assignment Aug 7, 2019
@easwars easwars merged commit cd5357d into grpc:master Aug 7, 2019
@menghanl menghanl changed the title transport: Change Unlock to defer Unlock, to avoid data race transport: call Unlock in defer to avoid data race Aug 8, 2019
@menghanl menghanl added this to the 1.23 Release milestone Aug 8, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants