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

Jump to bottom

net/http: Avoid copying URL in Request.WithContext #52239

Closed
bpowers opened this issue Apr 8, 2022 · 1 comment
Closed

net/http: Avoid copying URL in Request.WithContext #52239

bpowers opened this issue Apr 8, 2022 · 1 comment
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance

Comments

@bpowers
Copy link
Contributor

bpowers commented Apr 8, 2022 •
edited

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

$ go version
go version devel go1.19-3e387528e5 Fri Apr 8 14:13:32 2022 +0000 darwin/arm64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/bpowers/Library/Caches/go-build"
GOENV="/Users/bpowers/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/bpowers/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/bpowers"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/bpowers/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/bpowers/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="devel go1.19-3e387528e5 Fri Apr 8 14:13:32 2022 +0000"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/bpowers/go/src/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/8t/qj6z573x0gvdm6s6krh_9qvw0000gn/T/go-build1085205980=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

A common pattern in HTTP middleware is to compute a value, derive a new context with Context.WithValue (or WithDeadline, etc), attach the context to the request using WithContext, and invoke the next handler (https://goji.io/ in particular encourages this pattern for middleware, but I think it is generally common).

WithContext has a TODO around trying to remove the not-quite-deep-but-not-quite-shallow behavior of copying the URL now that additional NewRequestWithContext and Clone methods exist for http.Requests:

func (r *Request) WithContext(ctx context.Context) *Request {
	if ctx == nil {
		panic("nil context")
	}
	r2 := new(Request)
	*r2 = *r
	r2.ctx = ctx
	r2.URL = cloneURL(r.URL) // legacy behavior; TODO: try to remove. Issue 23544
	return r2
}

I'd like to remove this copy, as users now have Clone to get real deep-copy semantics.

What did you expect to see?

One less allocation in this common hot-path function.

What did you see instead?

An allocation for legacy behavior with a TODO to remove it.
@gopherbot
Copy link

Change https://go.dev/cl/399155 mentions this issue: net/http: remove cloneURL call in WithContext

@seankhliao seankhliao added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 8, 2022
@golang golang locked and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bpowers @gopherbot @seankhliao