Skip to content

x/net/proxy: TestDial persistently failing on js-wasm builder #32842

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.

Sign up for GitHub

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

Jump to bottom
Closed
bcmills opened this issue Jun 28, 2019 · 8 comments
Closed

x/net/proxy: TestDial persistently failing on js-wasm builder #32842

bcmills opened this issue Jun 28, 2019 · 8 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Jun 28, 2019 •
edited
Loading

The test for golang.org/x/net/proxy is persistently failing on the js-wasm builder.

If it is not expected to pass, it should be skipped to avoid masking more serious bugs.

https://build.golang.org/log/c1cfc25dcb267d98777f1b08f6f3842f329d4fa1:

--- FAIL: TestDial (0.02s)
    --- FAIL: TestDial/DirectWithCancel (0.00s)
        dial_test.go:35: dial tcp :1: Connection refused
    --- FAIL: TestDial/DirectWithTimeout (0.00s)
        dial_test.go:54: dial tcp :3: Connection refused
FAIL
FAIL	golang.org/x/net/proxy	4.817s
@bcmills bcmills changed the title x/net/proxy: x/net/proxy: TestDial persistently failing on js-wasm builder Jun 28, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Jun 28, 2019

CC @dweomer @bradfitz @neelance

@bcmills bcmills added arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. labels Jun 28, 2019
@bcmills bcmills added this to the Unreleased milestone Jun 28, 2019
@bcmills bcmills mentioned this issue Jun 28, 2019
Open
@agnivade
Copy link
Contributor

agnivade commented Jun 28, 2019 •
edited
Loading

I think they should be skipped. Dial tests are not supported in js. As is evident from the build tag in net/dial_test.go

@dweomer
Copy link

dweomer commented Jun 28, 2019

@agnivade wrote:

I think they should be skipped. Dial tests are not supported in js. As is evident from the build tag in net/dial_test.go

thanks for this pointer, i will submit a fix

@dweomer
Copy link

dweomer commented Jun 28, 2019 •
edited
Loading

So I found

go/src/net/dial_test.go

Line 5 in ef9217e

// +build !js
which added the skip when wasm/js support was added last year-ish. Looking over x/net it seems there should be more failures. What is interesting is that TestDial() is only failing with Direct* passes that actually attempt a connection to localhost via IP. The internal/socks stuff should fail as well but it is using nettest#NewLocalListener.

@agnivade
Copy link
Contributor

My guess is that is because sockstest.Server.TargetAddr returns a fake address.

// TargetAddr returns a fake final destination address.
//
// The returned address is only valid for testing with Server.
func (s *Server) TargetAddr() net.Addr {
	a := s.ln.Addr()
	switch a := a.(type) {
	case *net.TCPAddr:
		if a.IP.To4() != nil {
			return &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1), Port: 5963}
		}
		if a.IP.To16() != nil && a.IP.To4() == nil {
			return &net.TCPAddr{IP: net.IPv6loopback, Port: 5963}
		}
	}
	return nil
}

You will see in the tests that the port returned is always 5963.

In general, net.Listen at a kernel assigned port does not work in wasm

func main() {
	l, err := net.Listen("tcp4", "127.0.0.1:0")
	if err != nil {
		panic(err)
	}
	defer l.Close()
	fmt.Println(l.Addr().String())
}

gives - 127.0.0.1:1

So anything that dials at a listener listening at 127.0.0.1:0 will fail.

@dweomer
Copy link

dweomer commented Jun 28, 2019 •
edited
Loading

Turns out that not fully honoring the listener address, aka knowing it is a "local" address and so providing an empty string for the host portion of the address to Dial was the cause of this problem. I will submit my fix as it is more explicitly correct but this seems like a gap in the wasm/js networking code?

dweomer added a commit to dweomer/golang-net that referenced this issue Jun 28, 2019
@dweomer
proxy: fix TestDial failures on wasm/js

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
8f2967f 
Fixes golang/go#32842
@dweomer dweomer mentioned this issue Jun 28, 2019
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/184178 mentions this issue: proxy: fix TestDial failures on wasm/js

@dweomer
Copy link

dweomer commented Jun 28, 2019 

$ GOOS=js GOARCH=wasm go test -v ./proxy/...
=== RUN   TestDial
=== RUN   TestDial/DirectWithCancel
=== RUN   TestDial/DirectWithTimeout
=== RUN   TestDial/DirectWithTimeoutExceeded
=== RUN   TestDial/SOCKS5
=== RUN   TestDial/SOCKS5WithTimeout
=== RUN   TestDial/SOCKS5WithTimeoutExceeded
--- PASS: TestDial (0.01s)
    --- PASS: TestDial/DirectWithCancel (0.00s)
    --- PASS: TestDial/DirectWithTimeout (0.00s)
    --- PASS: TestDial/DirectWithTimeoutExceeded (0.00s)
    --- PASS: TestDial/SOCKS5 (0.00s)
    --- PASS: TestDial/SOCKS5WithTimeout (0.00s)
    --- PASS: TestDial/SOCKS5WithTimeoutExceeded (0.00s)
=== RUN   TestPerHost
=== RUN   TestPerHost/Dial
=== RUN   TestPerHost/DialContext
--- PASS: TestPerHost (0.00s)
    --- PASS: TestPerHost/Dial (0.00s)
    --- PASS: TestPerHost/DialContext (0.00s)
=== RUN   TestFromEnvironment
--- PASS: TestFromEnvironment (0.00s)
=== RUN   TestFromURL
--- PASS: TestFromURL (0.00s)
=== RUN   TestSOCKS5
--- PASS: TestSOCKS5 (0.00s)
=== RUN   TestFromEnvironmentUsing
--- PASS: TestFromEnvironmentUsing (0.00s)
PASS
ok      golang.org/x/net/proxy  1.099s

@golang golang locked and limited conversation to collaborators Jun 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

proxy: fix TestDial failures on wasm/js dweomer/golang-net
4 participants
@dweomer @agnivade @bcmills @gopherbot