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.

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

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

Open
jbardin opened this Issue Mar 6, 2018 · 6 comments

Comments

Projects
None yet
7 participants
@jbardin
Copy link
Contributor

jbardin commented Mar 6, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

jbardin commented Mar 6, 2018

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 this to the Unplanned milestone Mar 7, 2018

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Mar 7, 2018

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 7, 2018

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

This comment has been minimized.

Copy link
Member

dsnet commented Mar 7, 2018

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

This comment has been minimized.

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 label Mar 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.