Skip to content

Commit

Permalink
net: delete TestListenCloseListen
Browse files Browse the repository at this point in the history
In CL 557177, I attempted to fix a logical race in this test (#65175).
However, I introduced a data race in the process (#65209).

The race was reported on the windows-amd64-race builder. When I tried
to reproduce it on linux/amd64, I added a time.Sleep in the Accept
loop. However, that Sleep causes the test to fail outright with
EADDRINUSE, which suggests that my earlier guess about the open Conn
preventing reuse of the port was, in fact, incorrect.

On some platforms we could instead use SO_REUSEPORT and avoid closing
the first Listener entirely, but that wouldn't be even remotely in the
spirit of the original test.

Since I don't see a way to preserve the test in a way that is not
inherently flaky / racy, I suggest that we just delete it. It was
originally added as a regression test for a bug in the nacl port,
which no longer exists anyway. (Some of that code may live on in the
wasm port, but it doesn't seem worth maintaining a flaky
port-independent test to maintain a regression test for a bug specific
to secondary platforms.)

Fixes #65209.
Updates #65175.

Change-Id: I32f9da779d24f2e133571f0971ec460cebe7820a
Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-race
Reviewed-on: https://go-review.googlesource.com/c/go/+/557536
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
  • Loading branch information
Bryan C. Mills authored and gopherbot committed Jan 22, 2024
1 parent e5eeadb commit 6e7aee5
Showing 1 changed file with 0 additions and 70 deletions.
70 changes: 0 additions & 70 deletions src/net/net_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"net/internal/socktest"
"os"
"runtime"
"sync"
"testing"
"time"
)
Expand Down Expand Up @@ -294,75 +293,6 @@ func TestPacketConnClose(t *testing.T) {
}
}

func TestListenCloseListen(t *testing.T) {
if testing.Short() {
t.Parallel()
}

ln := newLocalListener(t, "tcp")
addr := ln.Addr().String()

var wg sync.WaitGroup
wg.Add(1)
go func() {
for {
c, err := ln.Accept()
if err != nil {
wg.Done()
return
}
wg.Add(1)
go func() {
io.Copy(io.Discard, c)
c.Close()
wg.Done()
}()
}
}()
defer wg.Wait()

// Keep a connection alive while we close the listener to try to discourage
// the kernel from reusing the listener's port for some other process.
//
// TODO(bcmills): This empirically seems to work, and we also rely on it in
// TestDialClosedPortFailFast, but I can't find a reference documenting this
// port-reuse behavior.
c, err := Dial("tcp", addr)
defer c.Close()

if err := ln.Close(); err != nil {
if perr := parseCloseError(err, false); perr != nil {
t.Error(perr)
}
t.Fatal(err)
}

if !testing.Short() {
// Burn through some ephemeral ports (without actually accepting any
// connections on them) to try to encourage the kernel to reuse the address
// if it is going to.
lns := make(chan []Listener, 1)
lns <- nil
for i := 0; i < 4000; i++ {
ln := newLocalListener(t, "tcp")
lns <- append(<-lns, ln)
}
defer func() {
for _, ln := range <-lns {
ln.Close()
}
}()
}

ln, err = Listen("tcp", addr)
if err == nil {
// Success. (This test didn't always make it here earlier.)
ln.Close()
return
}
t.Fatalf("failed to listen/close/listen on same address")
}

// See golang.org/issue/6163, golang.org/issue/6987.
func TestAcceptIgnoreAbortedConnRequest(t *testing.T) {
switch runtime.GOOS {
Expand Down

0 comments on commit 6e7aee5

Please sign in to comment.