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: should TeeReader return an io.ReadCloser? #27617

Open
michael-schaller opened this issue Sep 11, 2018 · 7 comments
Open

io: should TeeReader return an io.ReadCloser? #27617

michael-schaller opened this issue Sep 11, 2018 · 7 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. v2 A language change or incompatible library change
Milestone

Comments

@michael-schaller
Copy link
Contributor

io.TeeReader can't be used to wrap an io.ReadCloser as that strips the io.Closer part.

io.TeeReader could return an io.ReadCloser though as it could implement the Close method by calling the Close method of the given reader if it is an io.ReadCloser. The question is just what should happen if the given reader is just an io.Reader. In that case I would propose to just do nothing on Close method call as the wrapped io.Reader doesn't need to be closed.

IMHO this shouldn't violate the Go 1 compatibility as io.ReadCloser includes io.Reader and so all existing code should continue to work.

Thoughts? I'm happy to send a pull request.

@mvdan
Copy link
Member

mvdan commented Sep 11, 2018

Sorry, but this can't be done in a backwards compatible way. The following code would break:

var f func(io.Reader, io.Writer) io.Reader = io.TeeReader

I'm also unsure if this is a good idea, even for Go2. It would complicate the API quite a bit, when most users don't care about close errors when using an io.Reader.

TeeReader also seems simple enough, so it should be easy to just copy the code and adapt it to your needs.

@mvdan mvdan changed the title Should io.TeeReader return an io.ReadCloser? io: should TeeReader return an io.ReadCloser? Sep 11, 2018
@mvdan mvdan added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 11, 2018
@michael-schaller
Copy link
Contributor Author

michael-schaller commented Sep 11, 2018

@mvdan You make a good point that this would indeed break the Go 1 compatibility. I didn't think about that loophole. sigh

Can we then have an io.TeeReadCloser instead or an io.NewReadCloser to create a ReadCloser from a Reader and Closer? The later could be used like this: io.NewReadCloser(io.TeeReader(r, w), r) to achieve the same as an io.TeeReadCloser.

@mvdan
Copy link
Member

mvdan commented Sep 11, 2018

I don't think your last suggestion is worth adding to the io package. It's trivial to do with user code:

type readCloser struct {
    io.Reader
    io.Closer
}

func useTeeReader(rc io.ReadCloser, w io.Writer) {
    tee := io.TeeReader(rc, w)
    teeCloser := readCloser{tee, rc}
    useAndClose(teeCloser)
}

@bcmills bcmills added the v2 A language change or incompatible library change label Sep 11, 2018
@bcmills
Copy link
Member

bcmills commented Sep 11, 2018

io.teeReader could in theory implement io.Closer even if we didn't change the signature of the io.TeeReader function.

Taken to the logical extreme, though, that pattern would either require compile-time metaprogramming or result in an explosion of Reader implementations: there are other methods (such as WriteTo) that could/should be supported, and for n orthogonal wrappers we currently need 2ⁿ wrappers.¹

¹ See, for example, this experience report.

@carlmjohnson
Copy link
Contributor

What if there were io.AutoCloser(rc io.ReadCloser) io.Reader? Implementation could be this trivial:

type autocloser struct {
	rc io.ReadCloser
}

func (ac autocloser) Read(p []byte) (n int, err error) {
	n, err = ac.rc.Read(p)
	if err != nil {
		_ = ac.rc.Close()
	}
	return
}

@mvdan
Copy link
Member

mvdan commented Nov 13, 2018

@carlmjohnson I don't think the io package should be encouraging that. Closing a reader is more than just reaching EOF or an error. What about the cases where one wants to close before reaching EOF? What about when one wants to read a reader's contents multiple times via io.Seeker?

@carlmjohnson
Copy link
Contributor

In those cases, don't use the autocloser? :-) I think reading to io.EOF or first error is a pretty common case, but obviously, there are a lot of times when it doesn't apply and you shouldn't use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

5 participants