compiler/natives/src/net/http: Implement http.RoundTripper on Fetch API. #454

dmitshur · 2016-05-08T23:13:02Z

This is a WIP PR in order to get initial review comments, to communicate and avoid duplicate efforts. Known TODOs are listed in code.

The motivation to use Fetch API is because it's lower level, and allows implementing an http.RoundTripper more efficiently (less conversions between strings and []byte). It also supports streaming body responses efficiently.

References:

Tested in stable channel of Chrome, latest Safari, Firefox (developer edition).

Dynamically determine which of XHR, Fetch APIs are available and gracefully fallback to the best available API.

Here's a quick demo, using a local instance of https://http2.golang.org/reqinfo and https://http2.golang.org/clockstream endpoints. I've asked @bradfitz if it's possible to enable CORS on http2.golang.org itself, and if so, you could try this out in a playground.

func streamingDemo() error {
    resp, err := http.Get("https://localhost:4430/clockstream")
    if err != nil {
        return err
    }
    defer resp.Body.Close()
    _, err = io.Copy(os.Stdout, resp.Body)
    return err
}

Results

(It's a video, click it to view.)

dmitshur · 2016-05-10T08:02:00Z

Ok, I've setup an instance of h2demo with CORS enabled, so now it's possible to try out this change in the GopherJS Playground:

http://www.gopherjs.org/playground/#/5NfiqqGZCs

Wait for it to load, press run, and wait a few seconds as it compiles the program and runs it!

It will make a request to /reqinfo, then another to /clockstream and copy (streaming) first 1500 bytes before returning.

It should work in latest stable Chrome and any other browser that supports Fetch and Streams APIs (see http://caniuse.com/#search=Fetch).

neelance · 2016-05-10T09:36:50Z

Nice!

dmitshur · 2016-05-17T07:59:08Z

compiler/natives/net/http/fetch.go

+			header := Header{}
+			result.Get("headers").Call("forEach", func(value, key *js.Object) {
+				header[CanonicalHeaderKey(key.String())] = []string{value.String()} // TODO: Support multiple values.
+			})


Suggestions for how to do this better are very welcome.

What's a good way to iterate all keys (and values) of a JS Iterable?

In JS, one could do something like:

for (let value of ["a", "b", "c"]) { console.log(value); }

But what's a good way in GopherJS?

…API. The motivation to use Fetch API is because it's lower level, and allows implementing an http.RoundTripper more efficiently (less conversions between strings and []byte). It also supports streaming body responses efficiently. Includes known TODOs. References: - https://fetch.spec.whatwg.org/ - https://streams.spec.whatwg.org/ - http://caniuse.com/#search=Fetch Tested in stable channel of Chrome, latest Safari, Firefox (developer edition). Dynamically determine which of XHR, Fetch APIs are available and gracefully fallback to the best available API.

…lity. Add support for multiple same-key headers for request and response. Send request body. Close request body in RoundTrip to satisfy the interface contract. Also apply this fix to XHRTransport. Partial support for req.Cancel, remove CancelRequest(*http.Request) support since that method is deprecated. Fix a bug in streamReader due to not using pointer receiver Read method.

dmitshur · 2016-05-22T08:55:29Z

@neelance I've completed the critical functionality (closing response body, full request & response header support, and sending request body) in c11f3dd. PTAL.

This should be functionally ready to be merged. There are still some style and minor TODOs to clean up. Also, before merging, this should be squashed into logical commit(s) with clean commit message, and gopherjs/compiler/natives/fs_vfsdata.go needs to be regenerated.

@mholt

> But [Content-Length header] doesn't seem to be set even for non-streaming responses. That wasn't true. It is set for non-streaming responses. It's just that CORS needs to allow that header to be accessed by the frontend code, by setting w.Header().Set("Access-Control-Expose-Headers", "Content-Length"), etc. I wish we could use "redirect": "manual", since http.RoundTripper is not supposed to follow redirects, but that can't be done. Due to security reasons (/cc @mholt), browsers don't allow frontend code access to see intermediate redirect URLs, because that might allow for cross-site scripting attacks (according to https://fetch.spec.whatwg.org/#atomic-http-redirect-handling). XHRTransport suffers from the same problem/limitation. So, it's better to at least succeed and finish the request rather than not being able to follow redirects at all. The no body case is not applicable, I added that when I accidentally confused request body for response body. It should always be set since we're checking for ReadableStream support before opting in to use fetchTransport. If that's not the case, we'll need to adjust that logic.

This allows the browser implementation to return the underlying io.ReaderCloser directly, without having to first cache the entire thing. That enables streaming of the underlying source to be possible (which is possible in browser after Fetch Transport, see gopherjs/gopherjs#454. If seeking is desired, it can be added by the user on top of the returned io.ReadCloser. In many cases, seeking is not very critical and easy to avoid, and definitely not worth the expensive overhead of always doing supporting it. This is a slightly breaking API change if you depend on seeking, but easy to work around in client code.

This allows the browser implementation to return the underlying io.ReaderCloser directly, without having to first cache the entire thing. That enables streaming of the underlying source to be possible (which is possible in browser after Fetch Transport, see gopherjs/gopherjs#454. If seeking is desired, it can be added by the user on top of the returned io.ReadCloser. In many cases, seeking is not very critical and easy to avoid, and definitely not worth the expensive overhead of always supporting it. This is a slightly breaking API change if you depend on seeking, but easy to work around in client code.

neelance · 2016-05-22T21:53:33Z

compiler/natives/src/net/http/http.go

@@ -13,18 +13,29 @@ import (
 	"github.com/gopherjs/gopherjs/js"
 )

-var DefaultTransport RoundTripper = &XHRTransport{}
+var DefaultTransport = func() RoundTripper {
+	if fetchAPI, streamsAPI := js.Global.Get("fetch"), js.Global.Get("ReadableStream"); fetchAPI != js.Undefined && streamsAPI != js.Undefined {


Why the extra variables?

It checks that both fetch API and ReadableStream is available. The former is needed to make Fetch requests, and the latter is to be able to execute this line successfully:

Body: &streamReader{stream: result.Get("body").Call("getReader")},

For example, Firefox currently supports Fetch API but does not support streaming response bodies. So result.Get("body") would be nil, and using Fetch over XHR is not very advantageous.

See readonly attribute ReadableStream? body; at https://fetch.spec.whatwg.org/#response-class.

I meant this:

if js.Global.Get("fetch") != js.Undefined && js.Global.Get("ReadableStream") != js.Undefined {

Ah, I overlooked that or didn't think of it. It looks simpler, so I'll do it.

I think I initially tried to pass the variable into the transport, e.g.:

if xhrAPI := js.Global.Get("XMLHttpRequest"); xhrAPI != js.Undefined { return &XHRTransport{xhrAPI} }

But reverted because it was a bad idea. It meant that zero value was no longer valid, and there's no performance gain in storing js.Global.Get("XMLHttpRequest") in a variable.

It might also have been because I wanted to document the name of API being checked for.

neelance · 2016-05-22T21:58:27Z

compiler/natives/src/net/http/fetch.go

+		//       See BufferSource at https://fetch.spec.whatwg.org/#body-mixin.
+		body, err := ioutil.ReadAll(req.Body)
+		if err != nil {
+			req.Body.Close() // RoundTrip must always close the body, including on errors.


Can this be ensured via a defer at the beginning on the function?

Yes, but I don't think it'd be cleaner. You still have to check that req.Body is not nil before doing it.

There are no other returns before this section of code (the code above simply deals with setting up the fields needed for the request), so req.Body.Close() will always be called.

No need for fetchAPI, streamsAPI, xhrAPI local variables since the API names can be easily inferred from the JS object being checked.

dmitshur · 2016-05-22T22:27:43Z

In case it's helpful, I've tested the implementation significantly during development using https://github.com/shurcooL/play/blob/master/192/go-js-fetch/http/test/main.go. You can run that suite by go get -u -d github.com/shurcooL/play/192/go-js-fetch/http/test and gopherjs serveing it.

neelance · 2016-05-22T22:28:44Z

LGTM

It does not belong to `return &fetchTransport{}` line, so move it closer to the relevant line. Instead of putting it above, put it on the side. This is because this is an internal detail that you'll only care about if editing or trying to understand that line better. It's not needed for general readability. So having to scroll to the right to see it is very fitting IMO. This is modeled after Bret Victor's style of putting footnotes on the right hand side of the article (side notes?). They can be skipped unless the reader wants to learn more. See http://virtivia.com:27080/5o1yhnwvjrvw.png.

dmitshur · 2016-05-22T23:10:22Z

Squashed and merged as 00254cb and db27c7c (with logically separated changes, clean commit messages).

I did not force-push to the fetch-transport branch of this PR, so that we can still see the development history here. Instead, I just added "Closes #454." to the commit message of the squashed commit. But its content is identical to the diff in this PR.

dmitshur · 2016-05-23T00:42:12Z

For anyone interested in an updated demo, I've updated the playground to use latest GopherJS compiler and created this simple demo that uses http.Get:

http://www.gopherjs.org/playground/#/okdxAaGlfF

Followup to #454. Mention that Fetch API can be used. Also mention that by default, without third party polyfill modules for XHR or Fetch APIs, node.js will not support net/http.

Followup to #454. Mention that Fetch API can be used. Mention that only net/http client is supported, server is not. Also mention that by default, without third party polyfill modules for XHR or Fetch APIs, node.js will not support net/http.

This makes the behavior of http.DefaultTransport using Fetch API more similar as when using XHR API. It's unfortunate to have to do this, since it's kinda magic, but this seems to be the most reasonable default. It's consistent with the previous behavior of XHR implementation. There is no equivalent property in http.Request as far as I can tell. The only other way to set credentials would be to use req.AddCookie, but then one would need to get the cookie in JavaScript, which is not possible if that cookie has HttpOnly flag set. It appears this is an unfortunate reality that we have to deal with to provide a useful http.DefaultTransport implementation on frontend, because of restrictions put into place due to security concerns on the web. For reference, see: https://fetch.spec.whatwg.org/#concept-request-credentials-mode https://developer.mozilla.org/en-US/docs/Web/API/Request/credentials > A request has an associated credentials mode, which is "omit", > "same-origin", or "include". Unless stated otherwise, it is "omit". https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/withCredentials > Setting withCredentials has no effect on same-site requests. So default XHR behavior without withCredentials set is like Fetch with "same-origin" credentials mode. Updates #454.

dmitshur reviewed May 17, 2016
View reviewed changes

dmitshur added 2 commits May 21, 2016 23:36

dmitshur force-pushed the fetch-transport branch from 52c8b9e to c11f3dd Compare May 22, 2016 08:53

dmitshur changed the title ~~WIP: compiler/natives/net/http: Implement http.RoundTripper on Fetch API.~~ compiler/natives/net/http: Implement http.RoundTripper on Fetch API. May 22, 2016

dmitshur changed the title ~~compiler/natives/net/http: Implement http.RoundTripper on Fetch API.~~ compiler/natives/src/net/http: Implement http.RoundTripper on Fetch API. May 22, 2016

neelance reviewed May 22, 2016
View reviewed changes

Fix goroutine leak when cancelling request.

b1c7241

neelance reviewed May 22, 2016
View reviewed changes

Simplify DefaultTransport selection logic.

4f00876

No need for fetchAPI, streamsAPI, xhrAPI local variables since the API names can be easily inferred from the JS object being checked.

dmitshur closed this in db27c7c May 22, 2016

dmitshur deleted the fetch-transport branch May 22, 2016 23:13

dmitshur mentioned this pull request May 24, 2016

How do I debug native packages? #367

Closed

dmitshur added a commit that referenced this pull request Jun 12, 2016

doc: Update net/http notes.

c58dba4

Followup to #454. Mention that Fetch API can be used. Also mention that by default, without third party polyfill modules for XHR or Fetch APIs, node.js will not support net/http.

dmitshur mentioned this pull request Jun 12, 2016

doc: Update net/http notes. #476

Merged

dmitshur mentioned this pull request Jul 4, 2016

compiler/natives/src/net/http: Set Fetch credentials to same-origin. #486

Merged

dmitshur mentioned this pull request May 23, 2018

runtime: implement timers, JavaScript callbacks back into WebAssembly golang/go#25506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler/natives/src/net/http: Implement http.RoundTripper on Fetch API. #454

compiler/natives/src/net/http: Implement http.RoundTripper on Fetch API. #454

dmitshur commented May 8, 2016 •

edited

Loading

dmitshur commented May 10, 2016 •

edited

Loading

neelance commented May 10, 2016

dmitshur May 17, 2016

dmitshur commented May 22, 2016 •

edited

Loading

neelance May 22, 2016

dmitshur May 22, 2016 •

edited

Loading

neelance May 22, 2016

dmitshur May 22, 2016 •

edited

Loading

neelance May 22, 2016

dmitshur May 22, 2016 •

edited

Loading

neelance May 22, 2016

dmitshur commented May 22, 2016 •

edited

Loading

neelance commented May 22, 2016

dmitshur commented May 22, 2016 •

edited

Loading

dmitshur commented May 23, 2016 •

edited

Loading

compiler/natives/src/net/http: Implement http.RoundTripper on Fetch API. #454

compiler/natives/src/net/http: Implement http.RoundTripper on Fetch API. #454

Conversation

dmitshur commented May 8, 2016 • edited Loading

Results

dmitshur commented May 10, 2016 • edited Loading

neelance commented May 10, 2016

dmitshur May 17, 2016

Choose a reason for hiding this comment

dmitshur commented May 22, 2016 • edited Loading

neelance May 22, 2016

Choose a reason for hiding this comment

dmitshur May 22, 2016 • edited Loading

Choose a reason for hiding this comment

neelance May 22, 2016

Choose a reason for hiding this comment

dmitshur May 22, 2016 • edited Loading

Choose a reason for hiding this comment

neelance May 22, 2016

Choose a reason for hiding this comment

dmitshur May 22, 2016 • edited Loading

Choose a reason for hiding this comment

neelance May 22, 2016

Choose a reason for hiding this comment

dmitshur commented May 22, 2016 • edited Loading

neelance commented May 22, 2016

dmitshur commented May 22, 2016 • edited Loading

dmitshur commented May 23, 2016 • edited Loading

dmitshur commented May 8, 2016 •

edited

Loading

dmitshur commented May 10, 2016 •

edited

Loading

dmitshur commented May 22, 2016 •

edited

Loading

dmitshur May 22, 2016 •

edited

Loading

dmitshur May 22, 2016 •

edited

Loading

dmitshur May 22, 2016 •

edited

Loading

dmitshur commented May 22, 2016 •

edited

Loading

dmitshur commented May 22, 2016 •

edited

Loading

dmitshur commented May 23, 2016 •

edited

Loading