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.

Sign up for GitHub

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

Jump to bottom

io: Close and CloseWithError overwrite the error stored in a pipe #24283

Closed
jbardin opened this issue Mar 6, 2018 · 7 comments
Closed

io: Close and CloseWithError overwrite the error stored in a pipe #24283

jbardin opened this issue Mar 6, 2018 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@jbardin
Copy link
Contributor

jbardin commented Mar 6, 2018 •
edited
Loading

Calling Close after CloseWithError (or CloseWithError multiple times) overwrites the stored error in io.pipe.

Opening an issue for discussion first, since there is test coverage verifying this behavior, though it seems to only be there to check the atomic.Value, and it does subtly change the API.

Since pipes are often used concurrently, and multiple calls to Close usually have no effect, users would often expect the following code to be deterministic:

pr, pw := io.Pipe()        
go func() {
	defer pw.Close()
	_, err := io.Copy(pw, reader)
	if err != nil {
		pw.CloseWithError(err)
	}
	// more code
}()

But in this case, if there is an error and the PipeReader executes Read immediately after CloseWithError it will get the stored error from the Copy, but once defer pw.Close executes the error is set back to EOF.

Moving the error assignment into the pipe.once would make the above work, and probably also allow the removal of the sync.Value fields, since the errors are protected by the done chan and the sync.Once.
@dsnet
Copy link
Member

dsnet commented Mar 6, 2018

I'm fairly certain that this change will break many people. However, I do agree the proposed semantics is probably preferred. In generally, the first encountered error is more accurate than some later error. For example, errgroup stores the first error, not the most recent error.

probably also allow the removal of the sync.Value fields.

The readCloseError and writeCloseError methods have to read the other side's error value. The sync.Once provides synchronization for your own side, but provides no protection for the other side, so some form of race-safety is still needed.

@jbardin
Copy link
Contributor Author

jbardin commented Mar 6, 2018 •
edited
Loading

Hi @dsnet,

My hunch was that most code which is hitting this case is probably unknowingly broken, and this is in effect fixing it. I guess we need to decide whether this would be considered a bug or not.

Thanks, looking more closely I see that concurrent Read or Write calls still need mutual exclusion around the error values, but that doesn't really matter much for the discussion of the error semantics.

@andybons andybons added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 7, 2018
@andybons andybons added this to the Unplanned milestone Mar 7, 2018
@andybons
Copy link
Member

andybons commented Mar 7, 2018

/cc @ianlancetaylor what do you think? Proposal worthy or Go2?

@ianlancetaylor
Copy link
Member

I would like to understand why we can't just change the code today. Calling Close twice is an error. I don't see why the second call to Close should modify the error stored by the first call.

@dsnet
Copy link
Member

dsnet commented Mar 7, 2018 •
edited
Loading

Calling Close twice is an error.

Currently calling Close always return nil and is not documented as have one behavior or the other.

I can run a regression test to see measure the impact of this change.

@dsnet dsnet self-assigned this Mar 7, 2018
@rsc
Copy link
Contributor

rsc commented Mar 26, 2018

Only the first Close should have any effect. Anything else is a bug.

@spf13 spf13 added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 26, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 26, 2018
@jmartin82 jmartin82 mentioned this issue Jul 23, 2019
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/187197 mentions this issue: io: add error check on pipe close functions to avoid error overwriting

tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
@jmartin82 @tomocy
io: add error check on pipe close functions to avoid error overwriting
c8c9518 
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 golang#24283

Change-Id: Iefe8f758aeb775309424365f8177511062514150
GitHub-Last-Rev: b559540
GitHub-Pull-Request: golang#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>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@jmartin82 @t4n6a1ka
io: add error check on pipe close functions to avoid error overwriting

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
39aa887 
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 golang#24283

Change-Id: Iefe8f758aeb775309424365f8177511062514150
GitHub-Last-Rev: b559540
GitHub-Pull-Request: golang#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>
@frankywahl frankywahl mentioned this issue Nov 17, 2019
Closed
@golang golang locked and limited conversation to collaborators Aug 27, 2020
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

Successfully merging a pull request may close this issue.

io: add error check on pipe close functions to avoid error overwriting jmartin82/go
7 participants
@jbardin @rsc @spf13 @andybons @ianlancetaylor @dsnet @gopherbot