Skip to content

Automatically drain the body on close? #448

Open
@rw-access

Description

@rw-access

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:

type autodrainingReader struct {
	io.ReadCloser
}

func (a *autodrainingReader) Close() error {
	_ = io.Copy(ioutil.Discard, a.ReadCloser)
	return a.ReadCloser.Close()
}

// might not even be needed, looks like this is just for websockets
type autodrainingReadWriter struct {
	io.ReadWriteCloser
}

func (a *autodrainingReadWriter) Close() error {
	_ = io.Copy(ioutil.Discard, a.ReadWriteCloser)
	return a.ReadWriteCloser.Close()
}

func makeAutoDraining(r io.ReadCloser) io.ReadCloser {
	if rw, 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

  1. 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
  2. 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.

Update: discovered this golang/go@ce83415 via #123 (comment).

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions