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

Open
slinkydeveloper opened this issue Apr 22, 2020 · 3 comments
Open

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

slinkydeveloper opened this issue Apr 22, 2020 · 3 comments
Labels
Milestone

Comments

@slinkydeveloper
Copy link

@slinkydeveloper slinkydeveloper commented Apr 22, 2020

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

@andybons andybons commented Apr 22, 2020

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
@gopherbot gopherbot added the Proposal label Apr 22, 2020
@slinkydeveloper
Copy link
Author

@slinkydeveloper slinkydeveloper commented Apr 22, 2020

@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

@andybons andybons commented Apr 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.