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.

Sign up for GitHub

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

Jump to bottom

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
Go1.22

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

@seankhliao seankhliao added this to the Go1.22 milestone Dec 31, 2023
@seankhliao seankhliao added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Dec 31, 2023
@callthingsoff callthingsoff mentioned this issue Jan 1, 2024
Closed
@gopherbot
Copy link

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

@kleinkk76 kleinkk76 mentioned this issue Jan 1, 2024
Closed
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@callthingsoff @ezz-no
net/http: make Request.Clone create fresh copies for matches and othe…
23a5fef 
…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 <emmanuel@orijtech.com>
Run-TryBot: Jes Cok <xigua67damn@gmail.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.22
Development

Successfully merging a pull request may close this issue.

net/http: make Request.Clone create fresh copies for matches and otherValues callthingsoff/go
3 participants
@nussjustin @gopherbot @seankhliao