From ad96f3c1e5bee5ce0f2f5f2c413e0aead0a645ff Mon Sep 17 00:00:00 2001
From: Fabian Holler <fabian.holler@simplesurance.de>
Date: Mon, 31 Jul 2023 12:57:07 +0200
Subject: [PATCH] transport/http2server: wait in Close() until loopyWriter
 terminated

When Server.Stop() returns it is not guaranteed that the loopyWriter
go-routine terminated.

This can lead to a panic or a testing.(*common).logDepth() race
condition in Go Tests because t.Log is used after or during the testcase
terminates.
This can happen when:
- a GRPC server is started in a Go test,
- the GRPC logs are forwarded to t.Log,
- loopyWriter.run logs an error after server.Stop() and the Test
  method returns.

grpc@v1.57.0/internal/leakcheck is unable to detect that the
loopyWriter go-routine continues to run after server.Stop() because it
waits up to 10s for go-routines to terminate after a test finishes.
The loopyWriter returns much faster after Stop() returns.

To make server.Stop() wait until loopyWriter terminated:
- rename the existing writerDone field, which is only used in tests, to
  loopyWriterDone, the writerDone channel is closed when the loopyWriter
  go-routine exited
- change http2server.Close to wait until loopyWriterDone is closed
---
 internal/transport/http2_server.go   | 31 +++++++++++++++-------------
 internal/transport/transport_test.go |  2 +-
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/internal/transport/http2_server.go b/internal/transport/http2_server.go
index c48091f6c0a6..5e9657144cb4 100644
--- a/internal/transport/http2_server.go
+++ b/internal/transport/http2_server.go
@@ -68,18 +68,18 @@ var serverConnectionCounter uint64
 
 // http2Server implements the ServerTransport interface with HTTP2.
 type http2Server struct {
-	lastRead    int64 // Keep this field 64-bit aligned. Accessed atomically.
-	ctx         context.Context
-	done        chan struct{}
-	conn        net.Conn
-	loopy       *loopyWriter
-	readerDone  chan struct{} // sync point to enable testing.
-	writerDone  chan struct{} // sync point to enable testing.
-	remoteAddr  net.Addr
-	localAddr   net.Addr
-	authInfo    credentials.AuthInfo // auth info about the connection
-	inTapHandle tap.ServerInHandle
-	framer      *framer
+	lastRead        int64 // Keep this field 64-bit aligned. Accessed atomically.
+	ctx             context.Context
+	done            chan struct{}
+	conn            net.Conn
+	loopy           *loopyWriter
+	readerDone      chan struct{} // sync point to enable testing.
+	loopyWriterDone chan struct{}
+	remoteAddr      net.Addr
+	localAddr       net.Addr
+	authInfo        credentials.AuthInfo // auth info about the connection
+	inTapHandle     tap.ServerInHandle
+	framer          *framer
 	// The max number of concurrent streams.
 	maxStreams uint32
 	// controlBuf delivers all the control related tasks (e.g., window
@@ -257,7 +257,7 @@ func NewServerTransport(conn net.Conn, config *ServerConfig) (_ ServerTransport,
 		authInfo:          authInfo,
 		framer:            framer,
 		readerDone:        make(chan struct{}),
-		writerDone:        make(chan struct{}),
+		loopyWriterDone:   make(chan struct{}),
 		maxStreams:        maxStreams,
 		inTapHandle:       config.InTapHandle,
 		fc:                &trInFlow{limit: uint32(icwz)},
@@ -300,6 +300,7 @@ func NewServerTransport(conn net.Conn, config *ServerConfig) (_ ServerTransport,
 
 	defer func() {
 		if err != nil {
+			close(t.loopyWriterDone)
 			t.Close(err)
 		}
 	}()
@@ -339,7 +340,7 @@ func NewServerTransport(conn net.Conn, config *ServerConfig) (_ ServerTransport,
 		t.loopy = newLoopyWriter(serverSide, t.framer, t.controlBuf, t.bdpEst, t.conn, t.logger)
 		t.loopy.ssGoAwayHandler = t.outgoingGoAwayHandler
 		t.loopy.run()
-		close(t.writerDone)
+		close(t.loopyWriterDone)
 	}()
 	go t.keepalive()
 	return t, nil
@@ -1249,6 +1250,8 @@ func (t *http2Server) Close(err error) {
 		connEnd := &stats.ConnEnd{}
 		sh.HandleConn(t.ctx, connEnd)
 	}
+
+	<-t.loopyWriterDone
 }
 
 // deleteStream deletes the stream s from transport's active streams.
diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go
index 258ef7411cf0..24b7b1dc7bf7 100644
--- a/internal/transport/transport_test.go
+++ b/internal/transport/transport_test.go
@@ -1702,7 +1702,7 @@ func testFlowControlAccountCheck(t *testing.T, msgSize int, wc windowSizeConfig)
 	client.Close(errors.New("closed manually by test"))
 	st.Close(errors.New("closed manually by test"))
 	<-st.readerDone
-	<-st.writerDone
+	<-st.loopyWriterDone
 	<-client.readerDone
 	<-client.writerDone
 	for _, cstream := range clientStreams {