net: in TestNotTemporaryRead, do not assume that a dialed connection has been accepted

Previously, TestNotTemporaryRead issued the Read on the Accept side of
the connection, and Closed the Dial side. It appears that on some
platforms, Dial may return before the connection has been Accepted,
and if that connection is immediately closed with no bytes written and
SO_LINGER set to 0, the connection may no longer even exist by the
time Accept returns, causing Accept to block indefinitely until the
Listener is closed.

If we were to just swap the directions, we would have an analogous
problem: Accept could accept the connection and close it before the
client even finishes dialing, causing Dial (instead of Read) to return
the ECONNRESET error.

Here, we take a middle path: we Accept and Dial the connection
concurrently, but wait until both the Accept and the Dial have
returned (indicating that the connection is completely established and
won't vanish from the accept queue) before resetting the connection.

Fixes #29685
Updates #25289

Change-Id: Ida06f70f7205fffcdafa3df78bd56184e6cec760
Reviewed-on: https://go-review.googlesource.com/c/go/+/385314
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

1 files changed, 39 insertions, 25 deletions

diff --git a/src/net/net_test.go b/src/net/net_test.go
index 7b169916f1..76a9c8b151 100644
--- a/src/net/net_test.go
+++ b/src/net/net_test.go
@@ -9,7 +9,6 @@ package net
 import (
 	"errors"
 	"fmt"
-	"internal/testenv"
 	"io"
 	"net/internal/socktest"
 	"os"
@@ -515,35 +514,50 @@ func TestCloseUnblocksRead(t *testing.T) {
 
 // Issue 24808: verify that ECONNRESET is not temporary for read.
 func TestNotTemporaryRead(t *testing.T) {
-	if runtime.GOOS == "freebsd" {
-		testenv.SkipFlaky(t, 25289)
-	}
-	if runtime.GOOS == "aix" {
-		testenv.SkipFlaky(t, 29685)
-	}
 	t.Parallel()
-	server := func(cs *TCPConn) error {
-		cs.SetLinger(0)
-		// Give the client time to get stuck in a Read.
-		time.Sleep(50 * time.Millisecond)
+
+	ln := newLocalListener(t, "tcp")
+	serverDone := make(chan struct{})
+	dialed := make(chan struct{})
+	go func() {
+		defer close(serverDone)
+
+		cs, err := ln.Accept()
+		if err != nil {
+			return
+		}
+		<-dialed
+		cs.(*TCPConn).SetLinger(0)
 		cs.Close()
-		return nil
+
+		ln.Close()
+	}()
+	defer func() { <-serverDone }()
+
+	ss, err := Dial("tcp", ln.Addr().String())
+	if err != nil {
+		t.Fatal(err)
 	}
-	client := func(ss *TCPConn) error {
-		_, err := ss.Read([]byte{0})
-		if err == nil {
-			return errors.New("Read succeeded unexpectedly")
-		} else if err == io.EOF {
-			// This happens on Plan 9.
-			return nil
-		} else if ne, ok := err.(Error); !ok {
-			return fmt.Errorf("unexpected error %v", err)
-		} else if ne.Temporary() {
-			return fmt.Errorf("unexpected temporary error %v", err)
+	defer ss.Close()
+	close(dialed)
+	_, err = ss.Read([]byte{0})
+	if err == nil {
+		t.Fatal("Read succeeded unexpectedly")
+	} else if err == io.EOF {
+		// This happens on Plan 9, but for some reason (prior to CL 385314) it was
+		// accepted everywhere else too.
+		if runtime.GOOS == "plan9" {
+			return
 		}
-		return nil
+		// TODO: during an open development cycle, try making this a failure
+		// and see whether it causes the test to become flaky anywhere else.
+		return
+	}
+	if ne, ok := err.(Error); !ok {
+		t.Errorf("Read error does not implement net.Error: %v", err)
+	} else if ne.Temporary() {
+		t.Errorf("Read error is unexpectedly temporary: %v", err)
 	}
-	withTCPConnPair(t, client, server)
 }
 
 // The various errors should implement the Error interface.