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

net/http: Request.Clone() does not deep copy Body contrary to its docs (using GetBody works though) #36095

Open
nicolascouvrat opened this issue Dec 12, 2019 · 4 comments

Comments

@nicolascouvrat
Copy link

@nicolascouvrat nicolascouvrat commented Dec 12, 2019

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.

@ALTree ALTree changed the title http.Request.Clone()'s documentation is misleading (does not deep copy `Body`) net/http: http.Request.Clone()'s documentation is misleading (does not deep copy `Body`) Dec 12, 2019
@odeke-em odeke-em changed the title net/http: http.Request.Clone()'s documentation is misleading (does not deep copy `Body`) net/http: Request.Clone() does not deep copy Body contrary to its docs (using GetBody works though) Dec 14, 2019
@nicolascouvrat

This comment has been minimized.

Copy link
Author

@nicolascouvrat nicolascouvrat commented Dec 22, 2019

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.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Dec 24, 2019

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.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 24, 2019

Change https://golang.org/cl/212408 mentions this issue: net/http: document non-clonability of Body and Response

@nicolascouvrat

This comment has been minimized.

Copy link
Author

@nicolascouvrat nicolascouvrat commented Dec 24, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.