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

proposal: net/http: NewRequest should take url.URL as target #38583

Closed
slinkydeveloper opened this issue Apr 22, 2020 · 7 comments
Closed

proposal: net/http: NewRequest should take url.URL as target #38583

slinkydeveloper opened this issue Apr 22, 2020 · 7 comments

Comments

@slinkydeveloper
Copy link

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

$ go version
go version go1.14 linux/amd64

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="amd64"
GOBIN=""
GOCACHE="/home/slinkydeveloper/.cache/go-build"
GOENV="/home/slinkydeveloper/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/slinkydeveloper/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/lib/golang"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build012661714=/tmp/go-build -gno-record-gcc-switches"

What did you do?

NewRequestWithContext and NewRequest take string as target parameter. Then the target is internally parsed to url.URL using url.Parse().
Because a lot of applications out there pre-parse the url for several reasons, this API tends to have a negative impact to the applications. Some examples when this parsing is useless:

  • the url provided by another library
  • the target of the requests is always the same
  • the target is always an host with the same http scheme, so building the url manually is faster

In my personal opinion is also more "type safe" to require an url for the target parameter.

What did you expect to see?

A way to build a http.Request with a context without the overhead of stringifying the url and parsing it back.

Trying to work around the issue

I've tried to build manually the Request struct, but to provide a context i need to invoke WithContext that triggers a copy of the struct.

This is a benchmark of the two methods to build the Request:

var Req *http.Request

func BenchmarkNewRequest(b *testing.B) {
	ctx := context.TODO()
	for i := 0; i < b.N; i++ {
		Req, _ = http.NewRequestWithContext(ctx, "POST", "http://localhost/", nil)
	}
}

func BenchmarkNewRequestFunky(b *testing.B) {
	ctx := context.TODO()
	u, _ := url.Parse("http://localhost/")
	for i := 0; i < b.N; i++ {
		Req = &http.Request{}
		Req = Req.WithContext(ctx)
		Req.URL = u
		Req.Header = make(http.Header)
		Req.Host = u.Host
		Req.Method = "POST"
		Req.Proto = "HTTP/1.1"
		Req.ProtoMajor = 1
		Req.ProtoMinor = 1
	}
}
BenchmarkNewRequest-8        	 3054969	       417 ns/op	     432 B/op	       3 allocs/op
BenchmarkNewRequestFunky-8   	 5510088	       218 ns/op	     560 B/op	       3 allocs/op

NewRequest obviously allocates less because it doesn't trigger a copy of himself, but it's definitely slower. The reason why it's slower is dominated by the url.Parse, as these flamegraphs show:

Screenshot_2020-04-22 kncloudevents test cpu

Screenshot_2020-04-22 kncloudevents test cpu(1)

Possible solutions

  • ctx of the http.Request should be exported, in order to allow people to build manually the http.Request struct with url.URL and avoid the copying
  • NewRequest should take url.URL as target parameter, or a new method should be created that takes url.URL as target parameter
@andybons
Copy link
Member

We can’t change NewRequest as that would violate the Go 1 compatibility promise. Marking as a proposal to explore the other options:

  • ctx of the http.Request should be exported, in order to allow people to build manually the http.Request struct with url.URL and avoid the copying
  • a new method should be created that takes url.URL as target parameter

@andybons andybons changed the title net/http: NewRequest should take url.URL as target proposal: net/http: NewRequest should take url.URL as target Apr 22, 2020
@gopherbot gopherbot added this to the Proposal milestone Apr 22, 2020
@slinkydeveloper
Copy link
Author

@andybons I can provide a PR for one of the two solutions (IMO exporting the ctx is the best solution here)

@andybons
Copy link
Member

Let’s let the proposal review committee take an initial look first.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 6, 2021
@rsc
Copy link
Contributor

rsc commented Jul 28, 2021

We've been very careful not to export context.

r := new(Request).Clone(context) will allocate a request with the context you want,
and then you can fill out r's other fields. Or you can even do

r := (&Request{...}).Clone(context)

Both of these are smart enough not to allocate two requests: only the result of Clone is actually allocated, as the benchmark shows.

We already have too much API in net/http. I am skeptical this comes up enough to want/need more here.

@rsc
Copy link
Contributor

rsc commented Jul 28, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Jul 28, 2021
@rsc rsc moved this from Active to Likely Decline in Proposals (old) Aug 4, 2021
@rsc
Copy link
Contributor

rsc commented Aug 4, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Aug 11, 2021
@rsc
Copy link
Contributor

rsc commented Aug 11, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Aug 11, 2021
@golang golang locked and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

4 participants