From 5c865c78c378548c282b22d96e6d0375079d4417 Mon Sep 17 00:00:00 2001
From: "Bryan C. Mills" <bcmills@google.com>
Date: Mon, 17 Apr 2023 10:56:09 -0400
Subject: [PATCH] crypto/tls: retry DialWithTimeout until the listener accepts
 a connection

The point of DialWithTimeout seems to be to test what happens when the
connection times out during handshake. However, the test wasn't
actually verifying that the connection made it into the handshake at
all. That would not only fail to test the intended behavior, but also
leak the Accept goroutine until arbitrarily later, at which point it
may call t.Error after the test t is already done.

Instead, we now:

- retry the test with a longer timeout if we didn't accept a
  connection, and

- wait for the Accept goroutine to actually complete when the test
  finishes.

Fixes #59646.

Change-Id: Ie56ce3297e2c183c02e67b8f6b26a71e50964558
Reviewed-on: https://go-review.googlesource.com/c/go/+/485115
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Commit-Queue: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
---
 src/crypto/tls/tls_test.go | 70 +++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 23 deletions(-)

diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index d8a43add179..3e43a56f22a 100644
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -170,35 +170,59 @@ func TestDialTimeout(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping in short mode")
 	}
-	listener := newLocalListener(t)
 
-	addr := listener.Addr().String()
-	defer listener.Close()
+	timeout := 100 * time.Microsecond
+	for !t.Failed() {
+		acceptc := make(chan net.Conn)
+		listener := newLocalListener(t)
+		go func() {
+			for {
+				conn, err := listener.Accept()
+				if err != nil {
+					close(acceptc)
+					return
+				}
+				acceptc <- conn
+			}
+		}()
 
-	complete := make(chan bool)
-	defer close(complete)
-
-	go func() {
-		conn, err := listener.Accept()
-		if err != nil {
-			t.Error(err)
-			return
+		addr := listener.Addr().String()
+		dialer := &net.Dialer{
+			Timeout: timeout,
+		}
+		if conn, err := DialWithDialer(dialer, "tcp", addr, nil); err == nil {
+			conn.Close()
+			t.Errorf("DialWithTimeout unexpectedly completed successfully")
+		} else if !isTimeoutError(err) {
+			t.Errorf("resulting error not a timeout: %v\nType %T: %#v", err, err, err)
 		}
-		<-complete
-		conn.Close()
-	}()
 
-	dialer := &net.Dialer{
-		Timeout: 10 * time.Millisecond,
-	}
+		listener.Close()
 
-	var err error
-	if _, err = DialWithDialer(dialer, "tcp", addr, nil); err == nil {
-		t.Fatal("DialWithTimeout completed successfully")
-	}
+		// We're looking for a timeout during the handshake, so check that the
+		// Listener actually accepted the connection to initiate it. (If the server
+		// takes too long to accept the connection, we might cancel before the
+		// underlying net.Conn is ever dialed — without ever attempting a
+		// handshake.)
+		lconn, ok := <-acceptc
+		if ok {
+			// The Listener accepted a connection, so assume that it was from our
+			// Dial: we triggered the timeout at the point where we wanted it!
+			t.Logf("Listener accepted a connection from %s", lconn.RemoteAddr())
+			lconn.Close()
+		}
+		// Close any spurious extra connecitions from the listener. (This is
+		// possible if there are, for example, stray Dial calls from other tests.)
+		for extraConn := range acceptc {
+			t.Logf("spurious extra connection from %s", extraConn.RemoteAddr())
+			extraConn.Close()
+		}
+		if ok {
+			break
+		}
 
-	if !isTimeoutError(err) {
-		t.Errorf("resulting error not a timeout: %v\nType %T: %#v", err, err, err)
+		t.Logf("with timeout %v, DialWithDialer returned before listener accepted any connections; retrying", timeout)
+		timeout *= 2
 	}
 }