-
Notifications
You must be signed in to change notification settings - Fork 833
Conversation
w.pr cannot be used to indicate that the pipe has been closed. Instead, we use an atomic.Bool. The problem with using w.pr is that a write to it in the goroutine launched by writer.open can race with a read on w.pr in Close.
Note that it's still possible that w.pr.CloseWithError runs concurrently with w.pr.Close. But this is fine.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
richardartoul
left a comment
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, although wondering if we could just allow w.pr to be closed twice because it looks like thats concurrency safe:
if err == nil {
err = ErrClosedPipe
}
p.rerr.Store(err)
p.once.Do(func() { close(p.done) })
return nil
}
vangent
left a comment
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 see the race you are describing, but I don't think this PR completely fixes it. I think it changes the race from "might result in a nil access" to "might result in a double-Close".
It looks like double-Close is actually fine though (https://play.golang.com/p/WvceCRttXVU).
So I think it should be sufficient to drop the "prClosed" entirely, and just remove the "w.pr = nil" line (line 383). WDYT?
|
Ha, my review raced with @richardartoul :-). Glad we came to the same conclusion though. |
Codecov ReportAll modified and coverable lines are covered by tests
Additional details and impacted files@@ Coverage Diff @@
## master #3480 +/- ## ========================================== - Coverage 73.15% 73.14% -0.01% ========================================== Files 113 113 Lines 14984 14983 -1 ========================================== - Hits 10961 10960 -1 Misses 3260 3260 Partials 763 763 View full report in Codecov by Sentry. |
using w.pr is that a write to it in the goroutine launched by writer.open can
race with a read on w.pr in Close.
We just don't write to w.pr in writer.open. The read end of the pipe can still be
closed twice, but that's fine.
cbf04e9 to
08ff5df
Compare
|
Yea, I think it could double close the read end of the pipe before my change too, but it's just simpler to not use an atomic.Bool. Updated the pr, please take a look. |
|
Thanks! |