What version of Go are you using (go version)?
$ go version go version go1.13.5 linux/amd64
Does this issue reproduce with the latest release?
I am currently using the latest release afaik.
What operating system and processor architecture are you using (go env)?
go env Output
$ go env GO111MODULE="" GOARCH="amd64" GOBIN="" GOCACHE="/home/nicolascouvrat/.cache/go-build" GOENV="/home/nicolascouvrat/.config/go/env" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GONOPROXY="" GONOSUMDB="" GOOS="linux" GOPATH="/home/nicolascouvrat/go" GOPRIVATE="" GOPROXY="https://proxy.golang.org,direct" GOROOT="/usr/local/go" GOSUMDB="sum.golang.org" GOTMPDIR="" GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64" GCCGO="gccgo" AR="ar" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="/home/nicolascouvrat/go/src/github.sie.com/SIE-Private/navscan/go.mod" CGO_CFLAGS="-g -O2" CGO_CPPFLAGS="" CGO_CXXFLAGS="-g -O2" CGO_FFLAGS="-g -O2" CGO_LDFLAGS="-g -O2" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build112066667=/tmp/go-build -gno-record-gcc-switches" GOROOT/bin/go version: go version go1.13.5 linux/amd64 GOROOT/bin/go tool compile -V: compile version go1.13.5 uname -sr: Linux 5.0.0-37-generic Distributor ID: Ubuntu Description: Ubuntu 18.04.3 LTS Release: 18.04 Codename: bionic /lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Ubuntu GLIBC 2.27-3ubuntu1) stable release version 2.27. gdb --version: GNU gdb (Ubuntu 8.1-0ubuntu3.2) 8.1.0.20180409-git
What did you do?
When the bug happens, I was playing with httputil.DumpRequest, but I managed to reproduce it with a shorter example:
package main
import (
"bytes"
"context"
"encoding/json"
"fmt"
"net/http"
)
func main() {
data, err := json.Marshal(map[string]string{"key": "value"})
if err != nil {
fmt.Println("err")
}
c := &http.Client{}
req, err := http.NewRequest("POST", "https://postman-echo.com/post", bytes.NewBuffer(data))
if err != nil {
fmt.Println("err")
}
clone := req.Clone(context.TODO())
buf := make([]byte, 15)
n, err := clone.Body.Read(buf)
if err != nil {
fmt.Println(err)
}
fmt.Println(n, string(buf))
_, err = c.Do(req)
if err != nil {
fmt.Println("ERROR:", err)
}
}
What did you expect to see?
I expected Clone to return a deep copy, such as I can do whatever i want with the clone body without affecting the original request.
The documentation states:
Clone returns a deep copy of r with its context changed to ctx
What did you see instead?
The original request body is drained, and the code errors with
ERROR: Post https://postman-echo.com/post: http: ContentLength=15 with Body length 0
I think it comes from here where a shallow copy is done.
I feel like the reason why I want Clone to indeed be deep is not mainstream enough to warrant modifying Clone (I want to save the request object as it is in a Debug struct to inspect it later), and the cost could be big if the body is. However, I think it would be a good idea to update the documentation? I am happy to do a PR in this case.
EDIT: Doing:
clone.Body, err = req.GetBody()
if err != nil {
//
}
solves my initial problem, which was that httputil.DumpRequestOut() essentially destroyed the Body of the argument. Maybe that function should be updated to use GetBody() instead? I would be happy to make a second issue and PR for that one too.
I still think the documentation problem for Clone stands though.
Comment From: nicolascouvrat
What I did to solve it is use the same approach as in httputil.DumpRequest:
// in Clone
*r2 = *r
var b bytes.Buffer
b.ReadFrom(r.Body)
r.Body = ioutil.NopCloser(&b)
r2.Body = ioutil.NopCloser(bytes.NewReader(b.Bytes()))
To reiterate, I'm happy to submit a PR, whether it be documentation or code, but I would like to know which one first to avoid spending time on something not wanted! Let me know.
Comment From: odeke-em
Thank you for reporting this issue @nicolascouvrat and welcome to the Go project!
So, given that Request.Body is an io.ReadCloser, there aren't guarantees for being clonable or rewindable. For the longest time in Go, we didn't even have GetBody (introduced in Go1.8) and cloning a Request.Body was implicitly impossible.
What I think that we can do here is update the docs for Clone to indicate that Request.Body will not be cloned, but that GetBody can be used for this purpose.
Comment From: gopherbot
Change https://golang.org/cl/212408 mentions this issue: net/http: document non-clonability of Body and Response
Comment From: nicolascouvrat
Hi @odeke-em and thanks for the insight. I'll change the docs in the upcoming few days when I'll have time, I guess it will make for a good introduction/first PR for me :)
So, given that Request.Body is an io.ReadCloser, there aren't guarantees for being clonable or rewindable.
Could you elaborate a little? Sorry if this is obvious, but I am a little confused. Is cloning an io.ReadCloser not what we are doing in net.httputil?
From what I get of your comment (which makes a lot of sense, I've had a feeling that the httputil implementation of the cloning is clumsy), does that mean that the drainBody above should use GetBody() instead?
Thanks!
Comment From: talbspx
@nicolascouvrat i believe you are correct and the offending line should be - https://github.com/golang/go/blob/master/src/net/http/httputil/dump.go#L38
i think the solution is to return this instead return ioutil.NopCloser(bytes.NewReader(buf.Bytes())), ioutil.NopCloser(bytes.NewReader(buf.Bytes())), nil
Comment From: J7mbo
What I did to solve it is use the same approach as in
httputil.DumpRequest:``` // in Clone r2 = r
var b bytes.Buffer b.ReadFrom(r.Body) r.Body = ioutil.NopCloser(&b) r2.Body = ioutil.NopCloser(bytes.NewReader(b.Bytes())) ```
To reiterate, I'm happy to submit a PR, whether it be documentation or code, but I would like to know which one first to avoid spending time on something not wanted! Let me know.
Hi @nicolascouvrat. I am also trying to replace the context in a request. I tried your code in a middleware but it doesn't seem to copy the body:
if body, err := ioutil.ReadAll(r.Body); err == nil {
fmt.Println("BODY BEFORE: " + string(body)) // THIS PRINTS SOME JSON { .., }
}
r2 := r.Clone(ctx)
*r2 = *r
var b bytes.Buffer
b.ReadFrom(r.Body)
r.Body = ioutil.NopCloser(&b)
r2.Body = ioutil.NopCloser(bytes.NewReader(b.Bytes()))
if body, err := ioutil.ReadAll(r.Body); err == nil {
fmt.Println("AFTER: r2 BODY: " + string(body)) // DOES NOT PRINT ANY JSON
}
if body, err := ioutil.ReadAll(r2.Body); err == nil {
fmt.Println("AFTER: r BODY: " + string(body)) // DOES NOT PRINT ANY JSON
}
What am I missing?
Comment From: rhcarvalho
@J7mbo looks like your debugging code is the culprit. After you read the body the first time to print it, the second read reads nothing (all input was already consumed).
b.ReadFrom(r.Body)
^^ the body is empty after it has been read by ioutil.ReadAll.
Comment From: J7mbo
@rhcarvalho Thanks for finding that! That's a good few hours saved I reckon :) Sorry to hijack the thread!
Comment From: chennqqi
What I did to solve it is use the same approach as in
httputil.DumpRequest:``` // in Clone r2 = r
var b bytes.Buffer b.ReadFrom(r.Body) r.Body = ioutil.NopCloser(&b) r2.Body = ioutil.NopCloser(bytes.NewReader(b.Bytes())) ```
To reiterate, I'm happy to submit a PR, whether it be documentation or code, but I would like to know which one first to avoid spending time on something not wanted! Let me know.
you forgot r.Body.Close()?
Comment From: 3052
just ran into this. I agree that either Clone should clone the body, or it should be documented that it does not clone the body. as others said workarounds are GetBody or DumpRequest
Comment From: arvenil
The Request can be not only client request, but also server request. In case of the later the GetBody() is not set. IMHO r.Clone() could also set the GetBody if it's not already set, same as NewRequest does?
Comment From: gopherbot
Change https://go.dev/cl/593175 mentions this issue: net/http: document that Request.Clone does not deep copy Body