The current implementation allows multiple calls `Close` and `CloseWithError` in every side of the pipe, as a result, the original error can be overwritten.
This CL fixes this behavior adding an error existence check on `atomicError` type
and keeping the first error still available.
Fixes#24283
Change-Id: Iefe8f758aeb775309424365f8177511062514150
GitHub-Last-Rev: b559540d7a
GitHub-Pull-Request: golang/go#33239
Reviewed-on: https://go-review.googlesource.com/c/go/+/187197
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Usage of atomic.Value has a subtle requirement that the
value be of the same concrete type. In prior usage, the intention
was to consistently store a value of the error type.
Since error is an interface, the underlying concrete can differ.
Fix this by creating a type-safe abstraction over atomic.Value
that wraps errors in a struct{error} type to ensure consistent types.
Change-Id: Ica74f2daba15e4cff48d2b4f830d2cb51c608fb6
Reviewed-on: https://go-review.googlesource.com/75594
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
In the distant past, Pipe was implemented with channels and a
long running pipe.run goroutine (see CL 994043).
This approach of having all communication serialized through the
run method was error prone giving Pipe a history of deadlocks
and race conditions.
After the introduction of sync.Cond, the implementation was rewritten
(see CL 4252057) to use condition variables and avoid the
long running pipe.run goroutine. While this implementation is superior
to the previous one, this implementation is strange in that the
p.data field is always set immediately prior to signaling the other
goroutine with Cond.Signal, effectively making the combination of the
two a channel-like operation. Inferior to a channel, however, this still
requires explicit locking around the p.data field.
The data+rwait can be effectively be replaced by a "chan []byte" to
inform a reader that there is data available.
The data+wwait can be effectively be replaced by a "chan int" to
inform a writer of how many bytes were read.
This implementation is a simplified from net.Pipe in CL 37402.
Change-Id: Ia5b26320b0525934fd87a3b69a091c787167f5aa
Reviewed-on: https://go-review.googlesource.com/65330
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Since commit cc62bed0 (CL 994043) the pipe deadlock when doing
Read+Close or Write+Close on same end was fixed, alas with test for
Read+Close case only.
Then commit 6d6f3381 (CL 4252057) made a thinko: in the writer path
p.werr is checked for != nil and then err is set but there is no break
from waiting loop unlike break is there in similar condition for reader.
Together with having only Read+Close case tested that made it to leave
reintroduced Write+Close deadlock unnoticed.
Fix it.
Implicitly this also fixes net.Pipe to conform to semantic of net.Conn
interface where Close is documented to unblock any blocked Read or Write
operations.
No test added to net/ since net.Pipe tests are "Assuming that the
underlying io.Pipe implementation is solid and we're just testing the
net wrapping". The test added in this patch should be enough to cover
the breakage.
Fixes#18401
Updates #18170
Change-Id: I9e9460b3fd7d220bbe60b726accf86f352aed8d4
Reviewed-on: https://go-review.googlesource.com/34637
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Jordi Martin
Joe Tsai
Kirill Smelkov
Russ Cox