Go 1.19: request body is not seekable error

[barrettj12]

func test(url string, client *httpbakery.Client) {
	b := []byte{}
	req, _ := http.NewRequest("POST", url, bytes.NewReader(b))
	response, err := client.Do(req)
	fmt.Println(err)
}

This works fine on Go <= 1.18, but on Go 1.19, it is failing with the error

request body is not seekable

Looking into the code, it seems the issue is with the function httpbakery/request.go:seekerFromBody. In the Go 1.19 release notes:

NopCloser's result now implements WriterTo whenever its input does.

So the check here is not quite correct - we need to compare it against the type of io.nopCloserWriterTo.
I wonder if there's actually a more robust way to do this.

[benhoyt]

@barrettj12 Yeah, this is a tricky one. We could fix this library so it handles the new case: either by testing against a type that is nopCloserWriterTo, as you suggest, or by pulling out the first field in either case, and if it's called "Reader" and is of type io.Reader, use it. But that still seems hacky to me.

I wonder if the better fix is to read the request body into a []byte so we can just reuse it on retries. I did this recently for Charmhub API retries. See https://github.com/juju/juju/pull/14369/files#diff-5bbdbc4eb21ff45cba916c36e361b548308997f4798268d776c02d3ecb763731R139

[barrettj12]

I wonder if the better fix is to read the request body into a []byte so we can just reuse it on retries

Then we would have to implement Read, Seek, Close on the []byte right?

[benhoyt]

No, you can just wrap it in a bytes.Reader to get those. So in the code I linked I do this on every retry:

req.Body = io.NopCloser(bytes.NewReader(body))

[barrettj12]

Hmm, but then we're wrapping the bytes in a Reader, unwrapping, then wrapping again. Feels kinda pointless.

Looking at the io source code, it seems io.nopCloser and io.nopCloserWriterTo are the only options. I think the easiest and safest option is just to check against both types. I know this is "coding to the implementation" but I would expect the implementation to be fairly stable for the Go standard library.

[benhoyt]

It might be "pointless", but it's also very cheap (a bytes.Reader is simply a struct which references the underlying []byte).

However, I'm not opposed to the nopCloser + nopCloserWriterTo approach either. It's no worse than what we have now. :-)