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

Closed
michael-schaller opened this issue Sep 11, 2018 · 9 comments
Closed

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

michael-schaller opened this issue Sep 11, 2018 · 9 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
Contributor

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.

@earthboundkid
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?

@earthboundkid
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.

@ianlancetaylor
Copy link
Contributor

Per the discussion here, we can't change the current io.TeeReader and we don't want to try to handle all possible methods. TeeReader is a simple tool that doesn't even do any buffering, and people who need something more complex should use a more complex tool. Closing this issue.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2023
@chakrit
Copy link

chakrit commented Jan 18, 2024

The io that needs closing is pretty much everything unless you don't care about any IO leaks so I think this is kind of weird that TeeReader would not support it.

However, do understand that changing the current impl. would break a lot of things tho as the maintainers have said.

So just leaving this code here in case anyone needs it. I went over a few ways to do this and ended up with this which I think is pretty minimal and Go-ish (if that's a thing) and doesn't require you to restructure a ton of code.

Just throw a closeSegue whenever you have a io.ReadCloser and also needs a TeeReader in there somewhere.

type closeSegue struct {
	reader io.Reader
	closer io.Closer
}

func (r *closeSegue) Read(p []byte) (n int, err error) { return r.reader.Read(p) }
func (r *closeSegue) Close() error                     { return r.closer.Close() }

Here's an example of how I'm using it to "tee" incoming request body to stdout and still ensure that other middlewares down the line can still properly .Close() the body.

if config.Get(cfg, DebugRequestConfig) {
	tee := io.TeeReader(req.Body, debugOut)
	req.Body = &closeSegue{reader: tee, closer: req.Body}
}

next.ServeHTTP(resp, req)

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

7 participants