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: TCPListener fails to return errors after Close on js/wasm #50216

Open
bcmills opened this issue Dec 16, 2021 · 2 comments
Open

net: TCPListener fails to return errors after Close on js/wasm #50216

bcmills opened this issue Dec 16, 2021 · 2 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 16, 2021

The test in https://go.dev/play/p/TO_F52CqMt8 consistently passes locally.
However, it consistently fails under js/wasm (CC @neelance):

$ GOOS=js GOARCH=wasm go test -v
=== RUN   TestTCPListenAfterClose
    listener_test.go:51: after l.Close(), l.Accept() = _, <nil>
        want use of closed network connection
--- FAIL: TestTCPListenAfterClose (0.01s)
FAIL

As far as I can tell this violates the net.Listener interface, which requires (emphasis mine):

Close closes the listener.
Any blocked Accept operations will be unblocked and return errors.

I noticed this via #22926.

package main

import (
	"context"
	"errors"
	"net"
	"sync"
	"testing"
	"time"
)

func TestTCPListenAfterClose(t *testing.T) {
	l, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		t.Fatal(err)
	}

	var wg sync.WaitGroup
	ctx, cancel := context.WithCancel(context.Background())

	d := &net.Dialer{}
	for n := 2; n > 0; n-- {
		wg.Add(1)
		go func() {
			defer wg.Done()

			c, err := d.DialContext(ctx, l.Addr().Network(), l.Addr().String())
			if err == nil {
				<-ctx.Done()
				c.Close()
			}
		}()
	}

	c, err := l.Accept()
	if err == nil {
		c.Close()
	} else {
		t.Error(err)
	}
	time.Sleep(10 * time.Millisecond)
	cancel()
	wg.Wait()
	l.Close()

	c, err = l.Accept()
	if !errors.Is(err, net.ErrClosed) {
		if err == nil {
			c.Close()
		}
		t.Errorf("after l.Close(), l.Accept() = _, %v\nwant %v", err, net.ErrClosed)
	}
}
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 16, 2021

Change https://golang.org/cl/372714 mentions this issue: netutil: make LimitListener tests more robust

gopherbot pushed a commit to golang/net that referenced this issue Jan 5, 2022
In CL 372495 I cleaned up TestLimitListener so that it would not fail
spuriously. However, upon further thought I realized that the original
test was actually checking two different properties (steady-state
saturation, and actual overload), and the cleaned-up test was only
checking one of those (overload).

This change adds a separate test for steady-state saturation, and
makes the overload test more robust to spurious connections (which
could occur, for example, if another test running on the machine
accidentally dials this test's open port).

The test cleanup also revealed a bad interaction with an existing bug
in the js/wasm net.TCPListener implementation (filed as
golang/go#50216), for which I have added a workaround in
(*limitListener).Accept.

For golang/go#22926

Change-Id: I727050a8254f527c7455de296ed3525b6dc90141
Reviewed-on: https://go-review.googlesource.com/c/net/+/372714
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@bcmills
Copy link
Member Author

@bcmills bcmills commented Jan 6, 2022

From further testing in CL 372714, it appears that the client ends of the buffered connections are not closed until the server ends have been returned (and closed) by Accept.

If the server closes the listener without subsequently draining the Accept queue, that can cause the client sides of those pending connections to hang indefinitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants