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

net/http: no way to set Fetch options when making outgoing http calls with js/wasm #26769

Open
eginez opened this issue Aug 2, 2018 · 19 comments

Comments

@eginez
Copy link

@eginez eginez commented Aug 2, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version devel +c814ac44c0 Thu Jul 19 21:30:27 2018 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="wasm"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="js"
GOPATH="/Users/eginez/repos/goland"
GOPROXY=""
GORACE=""
GOROOT="/Users/eginez/repos/go"
GOTMPDIR=""
GCCGO="gccgo"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/pm/tlg5p5nx4vj3s05h4rfsv3t40000gn/T/go-build136489492=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The native http client, maps to the fetch() api. With the current client it is impossible to set additional options as supported by the fetch call, see: https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch.

What did you expect to see?

A way to specify options for the fetch call

@eginez eginez changed the title No way to set options when making outgoing http calls WASM Wasm: No way to set options when making outgoing http calls Aug 2, 2018
@eginez eginez changed the title Wasm: No way to set options when making outgoing http calls Wasm: No way to set fetch options when making outgoing http calls Aug 2, 2018
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 2, 2018

Yes, this is known. Currently, there is no good way to expose fetch parameters from user level to the http.RoundTripper which is what we are using to make the fetch call. It is not very straightforward, as we have to make some decisions on how best to integrate browser options into a Go API.

See a very small discussion here - https://go-review.googlesource.com/c/go/+/114515/8/src/net/http/roundtrip_js.go#34

The main tracking issue for a better wasm http client is #25695. I would suggest that you comment on that issue with your suggestions on how best we can take this forward.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Aug 2, 2018

@agnivade, things aren't that doomed. We can use http.Request.WithContext or HTTP headers to something to set options for Fetch.

/cc @neelance @dmitshur

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Aug 2, 2018

(That is, we don't need to wait for a new HTTP client package design to make progress on this)

@bradfitz bradfitz changed the title Wasm: No way to set fetch options when making outgoing http calls net/http: no way to set Fetch options when making outgoing http calls with js/wasm Aug 2, 2018
@bradfitz bradfitz added this to the Unplanned milestone Aug 2, 2018
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 2, 2018

Ah, using WithContext sounds like a good idea. I was overcomplicating things.

Also @johanbrandhorst

@johanbrandhorst

This comment has been minimized.

Copy link
Member

@johanbrandhorst johanbrandhorst commented Aug 2, 2018

That's a cool idea, but how do we make it obvious to the user? Is using the context as a transport for arbitrary downstream method invocations something well established? I'd be happy to implement this if there's a proposal.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Aug 2, 2018

Using contexts for options is actually generally considered a gross practice. But we can do so if needed.

But before designing anything, what are the specific option(s) we're talking about here? Maybe they already map to something we have in Go.

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 3, 2018

To me, mode and credentials are the 2 most important options that we should expose.

  • mode can take cors, no-cors, same-origin.
  • credentials can take omit, same-origin, include.

These are settings which arise only in a browser context. So as I see, there is no clear way to map them in Go.

We also have redirect which somehow maps with client.CheckRedirect.

See here for remaining options - https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch#Parameters

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Aug 3, 2018

Another option: we could use magic header values, like https://golang.org/pkg/net/http/#TrailerPrefix (it contains a colon which is illegal in general, so we special case its semantics).

@johanbrandhorst

This comment has been minimized.

Copy link
Member

@johanbrandhorst johanbrandhorst commented Aug 3, 2018

I have the beginnings of a CL ready for this. Will put it up later today.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 3, 2018

Change https://golang.org/cl/127718 mentions this issue: net/http: support configuring fetch options

@eginez

This comment has been minimized.

Copy link
Author

@eginez eginez commented Aug 3, 2018

@johanbrandhorst thanks for the quick turnaround on this issue. One quick comment I have on your PR. As a stop gap and until we can rehaul parts of the client, I see no problems with this approach. On the other hand I am really not inclined with the solution as presented if it this is going to be the long term answer. The idea that @bradfitz had about using context seems less magical to me. Thoughts?

@johanbrandhorst

This comment has been minimized.

Copy link
Member

@johanbrandhorst johanbrandhorst commented Aug 3, 2018

Please put comments on the CL: https://go-review.googlesource.com/c/go/+/127718

gopherbot pushed a commit that referenced this issue Aug 13, 2018
The default WASM RoundTripper is implemented using
the browser Fetch API. Some options don't readily map to
existing http.Request options, so we use the precedent
set by the TrailerPrefix constant to allow a user to configure
the "mode" and "credentials" options by supplying them
as headers in the http.Request.

Updates #26769

Change-Id: If42d24418c4ffb17211f57e36708cf460fb4c579
GitHub-Last-Rev: b230502
GitHub-Pull-Request: #26784
Reviewed-on: https://go-review.googlesource.com/127718
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@johanbrandhorst

This comment has been minimized.

Copy link
Member

@johanbrandhorst johanbrandhorst commented Aug 13, 2018

With #26784 merged we now have mode and credentials covered, so this issue is now about adding support for configuring redirect. Any more thoughts on how we might accomplish that?

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Aug 14, 2018

I don't see an obvious mapping from anything already in net/http to the Fetch redirect knob. So probably we should just do the same thing as the other two.

@johanbrandhorst

This comment has been minimized.

Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 3, 2019

I intend to submit a PR to add a way to configure the redirect behavior using the same mechanism as the others.

@johanbrandhorst

This comment has been minimized.

Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 3, 2019

@agnivade What other options do you think would be reasonable to expose via the header mechanism? I hesitate to close this issue until we cover all the existing options at least. Using your link we have:

Covered by existing API

method
headers
body
mode
credentials
redirect (soon)
signal (covered by context cancellation)

That leaves

cache: The cache mode you want to use for the request.

Not sure if there's some existing API we can map this to

referrer: A USVString specifying no-referrer, client, or a URL. The default is client.

Could presumably be configured via a header like the others.

referrerPolicy: Specifies the value of the referer HTTP header. May be one of no-referrer, no-referrer-when-downgrade, origin, origin-when-cross-origin, unsafe-url.

Could be configured via a header. This seems to affect the Referer header sent, which may already be configurable via existing API (unless that header is ignored when making the request).

integrity: Contains the subresource integrity value of the request (e.g., sha256-BpfBw7ivV8q2jLiT13fxDYAe2tJllusRSZ273h2nFSE=).

This is surely impossible for us to emulate? Or if we can figure out how this is calculated perhaps we could always send it?

keepalive: The keepalive option can be used to allow the request to outlive the page. Fetch with the keepalive flag is a replacement for the Navigator.sendBeacon() API. 

This too might map to some existing API?

Inputs appreciated.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 3, 2019

Change https://golang.org/cl/164666 mentions this issue: net/http: support configuring redirect fetch option

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Mar 4, 2019

For keepalive, we have this https://golang.org/pkg/net/http/#Transport.DisableKeepAlives. Is it possible to map to that ?

Other than that, I would just want to add cache. I have not used the other fields in practice. Will leave it to others to decide.

@johanbrandhorst

This comment has been minimized.

Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 4, 2019

I don't think they mean the same concept of keepalive. The Fetch keepalive sounds like an instruction to keep the request alive beyond the lifetime of the client (like a goroutine kind of?). I think HTTP keepalives are used to keep the connection alive. I might be wrong.

gopherbot pushed a commit that referenced this issue Mar 5, 2019
Adds a magic header value that is translated to the
Fetch API redirect option, following existing practices.

Updates #26769

Change-Id: Iaf1c9f710de63ea941a360b73f1b4bb725331a35
Reviewed-on: https://go-review.googlesource.com/c/go/+/164666
Reviewed-by: Richard Musiol <neelance@gmail.com>
Reviewed-by: Agniva De Sarker <agniva.quicksilver@gmail.com>
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.