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 clone path values #64911

Closed
nussjustin opened this issue Dec 31, 2023 · 2 comments
Closed

net/http: Request.Clone does not clone path values #64911

nussjustin opened this issue Dec 31, 2023 · 2 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@nussjustin
Copy link
Contributor

Go version

go version go1.22rc1 linux/amd64

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

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/justinn/.cache/go-build'
GOENV='/home/justinn/.config/go/env'
GOEXE=''
GOEXPERIMENT='loopvar'
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/justinn/.gopath/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/justinn/.gopath'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/justinn/sdk/go1.22rc1'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/justinn/sdk/go1.22rc1/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22rc1'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/justinn/Downloads/t/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1649121262=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I tried using the new Request.PathValue and Request.SetPathValue methods introduced in #61410 together with Request.Clone.

Code:

	orig, err := http.NewRequest("GET", "https://example.com/", nil)
	if err != nil {
		panic(err)
	}
	orig.SetPathValue("orig", "orig")

	copy := orig.Clone(context.Background())
	copy.SetPathValue("copy", "copy")

	fmt.Println(orig.PathValue("orig") == "orig") // true
	fmt.Println(orig.PathValue("copy") == "")     // false, should be true

	fmt.Println(copy.PathValue("orig") == "orig") // true
	fmt.Println(copy.PathValue("copy") == "copy") // true

Playground Link: https://go.dev/play/p/gz8wPsLmz6u?v=gotip

What did you expect to see?

Request.Clones documentation states

// Clone returns a deep copy of r with its context changed to ctx.

Thus I expect calls to SetPathValue on one request object to not affect the values returned by PathValue on the other request object.

What did you see instead?

Changes to the path values of one request also affect the other. See the example code and the playground example.

The reason for this is that Clone does not create copies of r.matches and r.otherValues.

@nussjustin
Copy link
Contributor Author

/cc @jba @neild

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/553375 mentions this issue: net/http: fix Request.Clone with SetPathValue

@dmitshur dmitshur moved this to In Progress in Release Blockers Jan 3, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Release Blockers Jan 3, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
…rValues

This change fixes Request.Clone to correctly work with SetPathValue
by creating fresh copies for matches and otherValues so that
SetPathValue for cloned requests doesn't pollute the original request.

While here, also added a doc for Request.SetPathValue.

Fixes golang#64911

Change-Id: I2831b38e135935dfaea2b939bb9db554c75b65ef
GitHub-Last-Rev: 1981db1
GitHub-Pull-Request: golang#64913
Reviewed-on: https://go-review.googlesource.com/c/go/+/553375
Reviewed-by: Emmanuel Odeke <[email protected]>
Run-TryBot: Jes Cok <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants