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.

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

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

Closed
bcmills opened this issue Dec 16, 2021 · 4 comments
Closed

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

bcmills opened this issue Dec 16, 2021 · 4 comments
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done. OS-JS
Milestone
Go1.22

Comments

@bcmills
Copy link
Contributor

bcmills commented Dec 16, 2021 •
edited

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)
	}
}
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-JS labels Dec 16, 2021
@bcmills bcmills added this to the Backlog milestone Dec 16, 2021
@gopherbot
Copy link

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

@bcmills bcmills mentioned this issue Dec 16, 2021
Closed
gopherbot pushed a commit to golang/net that referenced this issue Jan 5, 2022
netutil: make LimitListener tests more robust
5b0dc2d 
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
Contributor Author

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.

WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang
netutil: make LimitListener tests more robust
594f291 
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 bcmills mentioned this issue May 8, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/526117 mentions this issue: net: enable most tests on wasip1 and js

@bcmills bcmills added the arch-wasm WebAssembly issues label Sep 11, 2023
@bcmills bcmills modified the milestones: Backlog, Go1.22 Jan 20, 2024
@gopherbot
Copy link

Change https://go.dev/cl/557176 mentions this issue: net: remove an unused sync.Map in the fake net implementation

gopherbot pushed a commit that referenced this issue Jan 22, 2024
@gopherbot
net: remove an unused sync.Map in the fake net implementation
b17bf6d 
I added this map in CL 526117, but it is apparently unused.
I assume that I removed all uses of it while revising that change.

Updates #59718.
Updates #50216.

Change-Id: I8cdac39f4764d1fcc31566408304c850cf0f9374
Reviewed-on: https://go-review.googlesource.com/c/go/+/557176
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@ezz-no
net: remove an unused sync.Map in the fake net implementation
0f62033 
I added this map in CL 526117, but it is apparently unused.
I assume that I removed all uses of it while revising that change.

Updates golang#59718.
Updates golang#50216.

Change-Id: I8cdac39f4764d1fcc31566408304c850cf0f9374
Reviewed-on: https://go-review.googlesource.com/c/go/+/557176
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@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 Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done. OS-JS
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
3 participants
@dmitshur @bcmills @gopherbot