Conversation
|
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
richardartoul
left a comment
There was a problem hiding this comment.
LGTM, although wondering if we could just allow w.pr to be closed twice because it looks like thats concurrency safe:
func (p *pipe) closeRead(err error) error {
if err == nil {
err = ErrClosedPipe
}
p.rerr.Store(err)
p.once.Do(func() { close(p.done) })
return nil
}
vangent
left a comment
vangent
left a comment
There was a problem hiding this comment.
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. |
w.pr cannot be used to indicate that the pipe has been closed. 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. 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.
arjunnair1997
force-pushed
the
arjun/fix_data_race
branch
from
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! |
3 participants
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.