You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I just started using the module, and was a little surprised by the drain + close behavior.
I completely understand the need to always require res.Body.Close(), and that's pretty standard practice for Go. However, what seems a little unusual to me is that Close doesn't automatically do this for you.
I understand that there are some restrictions and that it's important to always read it, but it still looks a bit like a footgun. I want to remove this risk entirely from my codebase, since it's unlikely this be remembered at every call site (and frankly, it's too cumbersome). However, I do think it's more reasonable to expect defer resp.Body.Close() since that's consistent with every IO based library out there.
Two ways that first come to mind, would just need to make an io.Reader wrapper that can automatically drain itself on close:
typeautodrainingReaderstruct {
io.ReadCloser
}
func (a*autodrainingReader) Close() error {
_=io.Copy(ioutil.Discard, a.ReadCloser)
returna.ReadCloser.Close()
}
// might not even be needed, looks like this is just for websocketstypeautodrainingReadWriterstruct {
io.ReadWriteCloser
}
func (a*autodrainingReadWriter) Close() error {
_=io.Copy(ioutil.Discard, a.ReadWriteCloser)
returna.ReadWriteCloser.Close()
}
funcmakeAutoDraining(r io.ReadCloser) io.ReadCloser {
ifrw, ok:=r.(io.ReadWriteCloser); ok {
return&autodrainingReadWriter{rw}
}
return&autodrainingReader{r}
}
I think the question then comes down to where this goes. Two options come to mind
Wrap the transport http.RoundTripper so that it returns requests with auto-draining bodies. I'll test this out client-side, at least that gives me a workaround
Wrap the requests themselves as they are performed (more error prone)
What seems strange to me is that I see nothing in net/http that suggests the same issue where bodies must be read before they are closed (wrong: corrected below). But yet looking in the go-elasticsearch code base, I don't see anything that would challenge that either, so I'm not sure what the disclaimer is referring to.
That's an interesting idea, thank you for raising this.
We could implement it as an option as this hugely depends on how you built your consumption of the body, if any.
Let me know how your experiments are going, there are several options and one of them being not in the client but as you pointed in the transport.
I just started using the module, and was a little surprised by the drain + close behavior.
I completely understand the need to always require
res.Body.Close()
, and that's pretty standard practice for Go. However, what seems a little unusual to me is thatClose
doesn't automatically do this for you.I understand that there are some restrictions and that it's important to always read it, but it still looks a bit like a footgun. I want to remove this risk entirely from my codebase, since it's unlikely this be remembered at every call site (and frankly, it's too cumbersome). However, I do think it's more reasonable to expect
defer resp.Body.Close()
since that's consistent with every IO based library out there.Two ways that first come to mind, would just need to make an
io.Reader
wrapper that can automatically drain itself on close:I think the question then comes down to where this goes. Two options come to mind
http.RoundTripper
so that it returns requests with auto-draining bodies. I'll test this out client-side, at least that gives me a workaroundWhat seems strange to me is that I see nothing in net/http that suggests the same issue where bodies must be read before they are closed (wrong: corrected below). But yet looking in the
go-elasticsearch
code base, I don't see anything that would challenge that either, so I'm not sure what the disclaimer is referring to.Update: discovered this golang/go@ce83415 via #123 (comment).
The text was updated successfully, but these errors were encountered: