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, net/http, net/http/httptest: HTTP GET on local HTTP server fails with "fetch failed" on js/wasm with Node.js 18 #57613

Closed
dmitshur opened this issue Jan 4, 2023 · 8 comments
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done. OS-JS
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Jan 4, 2023

Some Go packages use a local HTTP server in examples. To make these work, the js/wasm port includes:

// Fake networking for js/wasm. It is intended to allow tests of other package to pass.

It works okay with Node.js 14, but fails with Node.js 18. Without it working, tests/examples in packages like compress/gzip and various others fail. This is the tracking issue this problem.

Tested at tip (79cdecc) with local patches to work around #56860 and #57516. Those issues will need to resolved first; I'm just reporting this finding earlier since I came across it while looking briefly into what's needed to make all.bash pass with Node 18.

It can be reproduced with GOOS=js GOARCH=wasm ./all.bash, or GOOS=js GOARCH=wasm go test -run='Example_compressingReader' compress/gzip, or with this more standalone program:

package main

import (
	"flag"
	"fmt"
	"io"
	"log"
	"net/http"
	"net/http/httptest"
	"os"
)

func main() {
	flag.Parse()

	err := run()
	if err != nil {
		log.Fatalln(err)
	}
}

func run() error {
	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
		fmt.Fprintln(w, "ok from httptest")
	}))
	defer ts.Close()
	fmt.Println("started a test HTTP server at:", ts.URL)

	resp, err := http.Get(ts.URL)
	if err != nil {
		return err
	}
	defer resp.Body.Close()
	fmt.Println(resp.Status)
	_, err = io.Copy(os.Stdout, resp.Body)
	return err

	// Output with Node.js 14:
	// 200 OK
	// ok from httptest

	// Output with Node.js 18:
	// started a test HTTP server at: http://127.0.0.1:1
	// (node:52134) ExperimentalWarning: The Fetch API is an experimental feature. This feature could change at any time
	// (Use `node --trace-warnings ...` to show where the warning was created)
	// 2023/01/04 16:40:18 Get "http://127.0.0.1:1": net/http: fetch() failed: fetch failed
	// exit status 1
}

CC @golang/js, @golang/wasm, @johanbrandhorst.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-wasm WebAssembly issues OS-JS labels Jan 4, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Jan 4, 2023
@dmitshur
Copy link
Contributor Author

Also CC @Pryz, @evanphx who've recently joined as Wasm maintainers in #57968. (Thanks!)
Investigating and resolving this issue will help unblock progress on #57614.

@johanbrandhorst
Copy link
Member

This is probably related to the new "experimental" fetch functionality in Node 18: https://nodejs.org/de/blog/announcements/v18-release-announce/#fetch-experimental.

@johanbrandhorst
Copy link
Member

johanbrandhorst commented Jan 28, 2023

OK I've confirmed that this is due to the introduction of the fetch API in Node 18. In Node 14, fetch was not defined

var jsFetchMissing = js.Global().Get("fetch").IsUndefined()

so we would short circuit the Fetch API roundtripper

if t.Dial != nil || t.DialContext != nil || t.DialTLS != nil || t.DialTLSContext != nil || jsFetchMissing {
return t.roundTrip(req)
}

This falls back to the fake network implementation in net_fake.go.

However, with Node 18, all of a sudden fetch is defined, so while on the server side we set up a fake in-memory listener, on the client side we try to make a request to the real local address. I see a few ways forward:

  1. Add a special case to disable the fetch API on NodeJS and reintroduce the roundtripper short circuit. We could use the new osinfo.Version() to determine the runtime environment (or just call to process directly). This would be unfortunate because it cripples Go wasm on NodeJS just so we can run the tests with a fake network.
  2. Implement a real NodeJS based socket syscall interface (based on https://nodejs.org/api/net.html presumably) so we can run proper networking tests for webassembly architectures (when run under NodeJS). This would probably be a lot of work. It's also not great because Go wasm on NodeJS is not supposed to be special, and we'd have to introduce runtime checks through the codebase to decide whether to lean on NodeJS APIs or return unimplemented errors. It's almost a js+node/wasm at that point.

I'm leaning towards option 1 for now as it means we can migrate to Node 18 with few changes and no net new functionality. At a later stage we could introduce a NodeJS based socket interface if we wanted to, and then reenable fetch for NodeJS. I will prepare a CL for option 1.

Would love @neelance's thoughts on a NodeJS based socket syscall interface, I assume the reason we didn't introduce one originally was because there was no fetch in NodeJS at the time.

Another note I want to quickly make here is that the NodeJS fetch implementation disallows the use of certain ports directly on the client (per the Fetch API spec). If we introduce fetch for NodeJS we have to ensure the server side tests don't use one of the blocked ports.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/463976 mentions this issue: net/http: disable fetch on NodeJS

@dmitshur
Copy link
Contributor Author

dmitshur commented Jan 30, 2023

Thanks for investigating this and figuring out the problem.

Would love @​neelance's thoughts on a NodeJS based socket syscall interface, I assume the reason we didn't introduce one originally was because there was no fetch in NodeJS at the time.

I asked Richard about that fairly recently, and he confirmed that the main reason the fake socket path was used is because that was the easiest path forward at the time. Back then, the NaCL port had the fake socket code implemented, and Node didn't support fetch. By now, with NaCL port being long gone and Node supporting fetch, it might be net simpler to start using fetch. But that is fine to investigate and consider doing if desired later.

Just disabling fetch during tests with Node 18 as done in CL 463976 is good to resolve the immediate issue. Thanks.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 30, 2023
johanbrandhorst added a commit to Pryz/go that referenced this issue Feb 12, 2023
NodeJS 18 introduced support for the fetch API for
making HTTP requests. This broke all wasm tests
that were relying on NodeJS falling back to the fake
network implementation in net_fake.go. Disable
the fetch API on NodeJS to get tests passing.

Fixes golang#57613

Change-Id: Icb2cce6d5289d812da798e07366f8ac26b5f82cb
Reviewed-on: https://go-review.googlesource.com/c/go/+/463976
Reviewed-by: Evan Phoenix <evan@phx.io>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/503675 mentions this issue: net/http: only disable Fetch API in tests

gopherbot pushed a commit that referenced this issue Jun 15, 2023
The Fetch API was meant to only be disabled in tests.
Since wasm_exec.js defines a global 'process' object,
it ended up being disabled anywhere that script is used.

Make the heuristic stricter so that it's less likely to
trigger anywhere but when testing js/wasm using Node.js.

For #57613.
Fixes #60808.

Change-Id: Ief8def802b466ef4faad16daccefcfd72e4398b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/503675
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
@brucealthompson
Copy link

brucealthompson commented Jan 13, 2024

I have done some testing and I definitely have this issue.

In fact, this is not an issue just for testing / playground applications. I have created a golang based webassembly module that runs on my website and uses the REST exported by that website. It does not work.

I have tested doing http gets in the webassembly to non locally based URLs. It works fine.

Is there any work around for the issue for someone using the golang http package?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/611215 mentions this issue: net/http: only disable fetch in test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done. OS-JS
Projects
None yet
Development

No branches or pull requests

4 participants