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: consider implementing browser http client at http.Client level instead of http.RoundTripper #25695

Open
dmitshur opened this Issue Jun 1, 2018 · 2 comments

Comments

Projects
None yet
1 participant
@dmitshur
Member

dmitshur commented Jun 1, 2018

The motivation for considering implementing the HTTP client at http.Client level rather than http.RoundTripper, it has to do with the fact that browser APIs (such as XmlHTTPRequest and Fetch) do not allow making individual HTTP requests without interpreting the response code, they operate at a higher level, closer to what http.Client.Do method does.

This issue is a continuation of a discussion in CL 114515. /cc @bradfitz @johanbrandhorst @neelance

I wrote:

http.Transport type is documented as:

// A Transport is a low-level primitive for making HTTP and HTTPS requests.
// For high-level functionality, such as cookies and redirects, see Client.

When I originally wrote this functionality in GopherJS, I started by doing it at the http.RoundTripper level and was hoping to implement as much of its semantics as possible. Getting response body streaming was my main motivation, and the XHR transport was already implemented at http.RoundTripper level too, so it was a quick decision.

Over time, I learned that both XHR and Fetch browser APIs are higher level and don't allow making individual HTTP requests. Due to security reasons and things like CORS, the frontend code doesn't actually ever get Redirect responses, only the final redirect destination. Fetch also deals with caching, credentials, etc.

I didn't want to touch this decision for GopherJS because it was more of a prototype, but since this is the real Go tree, perhaps we should revisit the decision of what abstraction level to implement this at. I haven't prototyped it yet, but given what I've seen so far, I suspect that implementing the HTTP client in browsers at the http.Client level may be a closer fit to what browsers allow client code to do.

Thoughts (Brad, Johan, Richard)?

@johanbrandhorst wrote:

I'm not sure I understand how this would apply here - wouldn't we still need a custom Transport implementation? If we defined a js,wasm specific client presumably it'd still need to have all the same fields as the normal client.

@bradfitz wrote:

Good point, but I'm more concerned with more code working by default without changes, so I think this is okay to violate for now in Go 1.11. We can revisit in Go 1.12 & later.

I've been thinking about this more, and I have 2 comments (to be posted as replies). It's not clear to me what will end up being the best (least bad) solution.

@dmitshur dmitshur added the Arch-Wasm label Jun 1, 2018

@dmitshur dmitshur added this to the Go1.12 milestone Jun 1, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Jun 1, 2018

wouldn't we still need a custom Transport implementation? If we defined a js,wasm specific client presumably it'd still need to have all the same fields as the normal client.

I think we'll need to think more about this and make some decisions. It's a fact that not all of the standard library functionality can be implemented in browsers, because in general browsers expose relatively high level APIs and not low level ones. As an example, the net package is not supported in browsers (there's no way to do low level TCP/UDP sockets in browsers), so if one tries to compile/run the following Go code in a browser:

conn, err := net.Dial("tcp", "golang.org:80")

It's clear that it can't work. But in what way would it not work? Viable choices are for net.Dial to always return a non-nil error, or the code can fail to compile.

This becomes trickier when you're dealing with a package like net/http, where a part of it should work, but a part of it cannot. We want code like this to work:

resp, err := http.Get("https://example.com/")

But we know the HTTP server can't be implemented inside the browser, so this Go code wouldn't work (either it always returns a non-nil error, or fails to compile because ListenAndServe identifier is missing for js/wasm):

err := http.ListenAndServe(":8080", nil)

The question that comes up is what to do when the Go code tries to use an http.RoundTripper in ways that the browser can't implement (e.g., trying to deal with redirect status code).

@dmitshur

This comment has been minimized.

Member

dmitshur commented Jun 1, 2018

I'm more concerned with more code working by default without changes, so I think this is okay to violate for now in Go 1.11. We can revisit in Go 1.12 & later.

I've recently realized an important data point regarding exactly this.

Right now, the net/http package exposes the low-level transport as an interface http.RoundTripper, but the high-level client as a concrete struct http.Client. As a consequence, it's very common to see Go code that overrides some behavior by passing around a pointer to an http.Client with a custom http.RoundTripper that wraps the underlying transport. It's not possible to override the http.Client behavior because it's not an interface (there's no Doer interface).

For example, here's Go code I wrote that works on frontend (via GopherJS) for creating an http.Client that performs authentication with an access token (i.e., it sets the "Authorization" header for each HTTP request):

import "golang.org/x/oauth2"

// httpClient gives an *http.Client for making API requests.
func httpClient() *http.Client {
	accessToken, err := ... // get it from a Cookie, if set
	if err != nil {
		// Non-authenticated client.
		return http.DefaultClient
	}
	// Authenticated client.
	src := oauth2.StaticTokenSource(
		&oauth2.Token{AccessToken: accessToken},
	)
	return oauth2.NewClient(context.Background(), src)
}

It would indeed nice to have code that like continue to work under js/wasm, and it requires being able to override the http.RoundTripper interface (since there's no other choice of an interface to override).

Would it be a good or bad idea to have a Doer interface, allowing such behavior to be implemented without dealing with the http.RoundTripper?

@dmitshur dmitshur changed the title from net/http: consider implementing http client at http.Client level instead of http.RoundTripper to net/http: consider implementing browser http client at http.Client level instead of http.RoundTripper Jun 1, 2018

@dmitshur dmitshur modified the milestones: Go1.12, Unplanned Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment