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

nicolascouvrat · 2019-12-12T03:07:06Z

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.

The text was updated successfully, but these errors were encountered:

nicolascouvrat · 2019-12-22T07:42:09Z

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 · 2019-12-24T04:18:57Z

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 · 2019-12-24T04:33:04Z

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

nicolascouvrat · 2019-12-24T08:42:08Z

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!

talbspx · 2020-07-19T17:43:19Z

@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

J7mbo · 2020-07-27T19:47:53Z

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?

rhcarvalho · 2020-07-27T20:00:04Z

@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.

J7mbo · 2020-07-27T20:02:03Z

@rhcarvalho Thanks for finding that! That's a good few hours saved I reckon :) Sorry to hijack the thread!

POST Bodyでも管理できるようにしました。今はcrawler側でPostFormのセットが必要ですが、可能なら無くしたいと思っています。

chennqqi · 2021-09-16T04:01:11Z

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()?

3052 · 2023-12-01T00:39:49Z

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

The Request.Clone() function is misleading, it does not create a copy of the original request's body, resulting in dumprequest+printf draining the original request's body. See golang/go#36095

## What this PR does / why we need it: The `Request.Clone()` function is misleading, it does not create a copy of the original request's body, resulting in dumprequest+printf draining the original request's body. See golang/go#36095 ## Which issue(s) this PR fixes: ## Special notes for your reviewer: ## Does this PR introduce a user-facing change? ``` No. ```

gopherbot added the Documentation label Dec 12, 2019

ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 12, 2019

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 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

JakeQuilty mentioned this issue Aug 5, 2020

Added OAuth1 functionality to the generic HTTP connector cyberark/secretless-broker#1321

Merged

8 tasks

j16r mentioned this issue Nov 11, 2020

Workaround for http.Request#Clone bug, fails to copy body smartcontractkit/chainlink#3619

Merged

t0mk mentioned this issue Dec 8, 2020

use http.Client instead of retryablehttp.Client packethost/packngo#222

Merged

y0d3n referenced this issue in After-the-CM/Himawari Sep 4, 2021

feat(sitemap): sitemapをPOSTに対応させた

4a2869c

POST Bodyでも管理できるようにしました。今はcrawler側でPostFormのセットが必要ですが、可能なら無くしたいと思っています。

Wwwsylvia mentioned this issue Jan 18, 2022

Fail to push images to private registries oras-project/oras-go#97

Closed

cnlangzi mentioned this issue Feb 28, 2022

feat/WriteMarkerMutex client 0chain/gosdk#393

Merged

3 tasks

budougumi0617 mentioned this issue May 18, 2022

20220526 Gophers Code Reading Party basebank/gophers-code-reading-party#30

Closed

seankhliao added this to the Unplanned milestone Aug 27, 2022

wmalik mentioned this issue Feb 5, 2024

fix: use req.GetBody when dumping req terramate-io/terramate#1422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nicolascouvrat commented Dec 12, 2019 •

edited

nicolascouvrat commented Dec 22, 2019

odeke-em commented Dec 24, 2019

gopherbot commented Dec 24, 2019

nicolascouvrat commented Dec 24, 2019

talbspx commented Jul 19, 2020

J7mbo commented Jul 27, 2020

rhcarvalho commented Jul 27, 2020

J7mbo commented Jul 27, 2020

chennqqi commented Sep 16, 2021

3052 commented Dec 1, 2023 •

edited

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

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

Comments

nicolascouvrat commented Dec 12, 2019 • edited

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

nicolascouvrat commented Dec 22, 2019

odeke-em commented Dec 24, 2019

gopherbot commented Dec 24, 2019

nicolascouvrat commented Dec 24, 2019

talbspx commented Jul 19, 2020

J7mbo commented Jul 27, 2020

rhcarvalho commented Jul 27, 2020

J7mbo commented Jul 27, 2020

chennqqi commented Sep 16, 2021

3052 commented Dec 1, 2023 • edited

nicolascouvrat commented Dec 12, 2019 •

edited

What version of Go are you using (`go version`)?

What operating system and processor architecture are you using (`go env`)?

3052 commented Dec 1, 2023 •

edited