blob/s3blob: fix data race

[arjunnair1997]

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.

[google-cla]

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]

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]

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?

[vangent]

Ha, my review raced with @richardartoul :-). Glad we came to the same conclusion though.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.14%. Comparing base (bb796f8) to head (08ff5df).
Report is 1 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

[arjunnair1997]

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.

[vangent]

Thanks!