internal/transport: close transport reader go routine before closing the connection #2

eshitachandwani · 2024-09-03T17:17:19Z

Fixes: grpc#2869
Previously, clientconn.Close() did not guarantee termination of reader goroutines upon return, as it only waited on channels like ctxDone, writerDone, and goAway.

Now, clientconn.Close() ensures that:

It waits for the readerDone channel to signal completion.
Transports are closed simultaneously, and clientconn.Close() waits for all transports to be closed before returning.

RELEASE NOTES:

internal/transport : Wait until all transports close and all go routines exit before clientconn.Close() returns

…exits

purnesh42H · 2024-09-09T04:06:34Z

Some mechanical comments

title of the PR should be in format: <package/area>: PR statement. In your case it could be internal/transport: close transport reader go routine before closing the connection
PR description should be in format: <fixes/address>: <#issue num>
With fix and without fix logs can be in a comment. Also, its better to just copy paste the logs instead of linking unless its too big
transport.close is not a function so let's not mention it that way. You can may be say clientconn.close()
Try to concise the PR description.

purnesh42H · 2024-09-09T04:07:30Z

.idea/workspace.xml

@@ -0,0 +1,63 @@
+<?xml version="1.0" encoding="UTF-8"?>


please remove. You can may be configure vscode to be ignored by git

clientconn.go

purnesh42H · 2024-09-09T04:26:44Z

internal/transport/http2_client.go

+	case <-t.readerDone:
+	//Signal received, proceed with closing
+	default:
+		//The signal has already been received, so don't wait again


this could be 2 cases right? 1) Either the close signal for readerDone was already consumed somewhere else or 2) the signal was never sent to channel?

i think we are sure we don't have 1) so its most likely 2)

Yes, but its only failing in the cases where t.close() is being called internally and also by the teardown, so it is closed once, and whenever it is closed, we stop waiting again by using the default case.

purnesh42H · 2024-09-09T04:36:51Z

internal/transport/http2_client.go

@@ -1023,6 +1023,12 @@ func (t *http2Client) Close(err error) {
 	case <-timer.C:
 		t.logger.Infof("Failed to write a GOAWAY frame as part of connection close after %s. Giving up and closing the transport.", goAwayLoopyWriterTimeout)
 	}
+	select {


we can just have single comment above select. Something like // wait for the reader channel to close before proceeding further

In the default case, we should probably log if running in a debug mode. Something similar to what we do in case of writerDone

if t.logger.V(logLevel) {
t.logger.Infof("Closing: %v", err)
}

dfawley · 2024-09-10T21:04:17Z

internal/transport/http2_client.go

@@ -1023,6 +1023,14 @@ func (t *http2Client) Close(err error) {
 	case <-timer.C:
 		t.logger.Infof("Failed to write a GOAWAY frame as part of connection close after %s. Giving up and closing the transport.", goAwayLoopyWriterTimeout)
 	}
+	// Wait for the reader channel to close
+	select {
+	case <-t.readerDone:


I think this needs to happen after the conn is closed, otherwise the reader will never exit.

dfawley · 2024-09-10T21:05:28Z

internal/transport/http2_client.go

+	case <-t.readerDone:
+	default:
+		if t.logger.V(logLevel) {
+			t.logger.Infof("The reader channel has already been closed: %v", err)


That's not what a default case means in a select with a channel.

I think we can pretty safely just close conn and do a blocking <-t.readerDone.

Yes, I did that when I did not know that the tests were failing when t.Close() was being called from inside reader function. I will remove it and refactor the code to make it work.

clientconn.go

dfawley · 2024-09-10T21:10:12Z

internal/transport/http2_client.go

+		if t.logger.V(logLevel) {
+			t.logger.Infof("The reader channel has already been closed: %v", err)
+		}
+	}


There is also potentially a keepalive goroutine that we need to wait for.

dfawley · 2024-09-16T22:07:23Z

clientconn.go

@@ -1110,6 +1110,7 @@ func (cc *ClientConn) ResetConnectBackoff() {

 // Close tears down the ClientConn and all underlying connections.
 func (cc *ClientConn) Close() error {
+	var wg sync.WaitGroup


Style: declare this immediately before it's needed.

dfawley · 2024-09-16T22:11:38Z

internal/transport/http2_client.go

@@ -1025,6 +1027,11 @@ func (t *http2Client) Close(err error) {
 	}
 	t.cancel()
 	t.conn.Close()
+	// Wait for the reader channel to close


Comments should explain why, not what. So this is // Wait for the reader goroutine to exit to ensure all resources are cleaned up before Close can return.. Similar below. Also make sure comments that are sentences are punctuated correctly (e.g. end with a period).

dfawley · 2024-09-16T22:16:15Z

internal/transport/http2_client.go

@@ -1627,12 +1632,12 @@ func (t *http2Client) readServerPreface() error {
 // reader verifies the server preface and reads all subsequent data from
 // network connection.  If the server preface is not read successfully, an
 // error is pushed to errCh; otherwise errCh is closed with no error.
-func (t *http2Client) reader(errCh chan<- error) {
+func (t *http2Client) readerUtil(errCh chan<- error) error {
 	defer close(t.readerDone)


Hmm, how about instead of another function and the awkward naming problem, do:

func (t ...) reader(....) { var err error defer func() { close(t.readerDone) if err != nil { t.Close(err) } }() // ... }

dfawley · 2024-09-16T22:16:34Z

internal/transport/http2_client.go

@@ -1684,7 +1688,12 @@ func (t *http2Client) reader(errCh chan<- error) {
 		case *http2.PingFrame:
 			t.handlePing(frame)
 		case *http2.GoAwayFrame:
-			t.handleGoAway(frame)
+			{


Why the code block here?

Because t.handleGoAway also calls t.close which again poses the same problem.

Then...I don't think this does what you think? A bare code block has no effect beyond scoping variables.

I didn't understand? I just want that t.Close should be called after the t.readerDone is closed. So, I just return the error from t.handleGoAway and call close when error!=null

dfawley · 2024-09-16T22:16:54Z

internal/transport/http2_client.go

+				err := t.handleGoAway(frame)
+				if err != nil {


These can be a compound if err := t.handle...(); err != nil { ... }

dfawley · 2024-09-16T22:18:04Z

internal/transport/http2_client.go

 // keepalive running in a separate goroutine makes sure the connection is alive by sending pings.
-func (t *http2Client) keepalive() {
+func (t *http2Client) keepaliveUtil() error {


Similar comment as above re: this pattern.

purnesh42H · 2024-09-17T15:01:28Z

internal/transport/http2_client.go

@@ -86,7 +86,8 @@ type http2Client struct {
 	writerDone chan struct{} // sync point to enable testing.
 	// goAway is closed to notify the upper layer (i.e., addrConn.transportMonitor)
 	// that the server sent GoAway on this transport.
-	goAway chan struct{}
+	goAway        chan struct{}
+	keepAliveDone chan struct{}


nit: comment for keepAliveDone channel

purnesh42H · 2024-09-17T15:02:15Z

internal/transport/http2_client.go

+	// Wait for the reader channel to close
+	<-t.readerDone
+	if t.keepaliveEnabled {
+		<-t.keepAliveDone //Wait for the keepAlive go routine to end


nit: space after //

eshitachandwani added 4 commits September 3, 2024 22:37

Waiting for the reader GO routine to end in conn.close()

3b026ee

Fix:waiting for reader go routine to complete before transport.close …

ad81690

…exits

close transport parallelly

e116ff9

remove comments

50b422a

purnesh42H reviewed Sep 9, 2024

View reviewed changes

resolve comments

27910f3

eshitachandwani changed the title ~~Waiting for the reader GO routine to end in transport.close()~~ internal/transport: close transport reader go routine before closing the connection Sep 10, 2024

dfawley reviewed Sep 10, 2024

View reviewed changes

Waiting for go and keepalive go routine

8388f02

dfawley reviewed Sep 16, 2024

View reviewed changes

purnesh42H reviewed Sep 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/transport: close transport reader go routine before closing the connection #2

internal/transport: close transport reader go routine before closing the connection #2

eshitachandwani commented Sep 3, 2024 •

edited

Loading

purnesh42H commented Sep 9, 2024

purnesh42H Sep 9, 2024

purnesh42H Sep 9, 2024 •

edited

Loading

purnesh42H Sep 9, 2024

eshitachandwani Sep 10, 2024

purnesh42H Sep 9, 2024

dfawley Sep 10, 2024

dfawley Sep 10, 2024

eshitachandwani Sep 13, 2024

dfawley Sep 10, 2024

dfawley Sep 16, 2024

eshitachandwani Sep 23, 2024

dfawley Sep 16, 2024

eshitachandwani Sep 23, 2024

dfawley Sep 16, 2024

dfawley Sep 16, 2024

eshitachandwani Sep 23, 2024

dfawley Sep 23, 2024

eshitachandwani Sep 24, 2024

dfawley Sep 16, 2024

dfawley Sep 16, 2024

eshitachandwani Sep 24, 2024

purnesh42H Sep 17, 2024

eshitachandwani Sep 24, 2024

purnesh42H Sep 17, 2024

eshitachandwani Sep 23, 2024

internal/transport: close transport reader go routine before closing the connection #2

Are you sure you want to change the base?

internal/transport: close transport reader go routine before closing the connection #2

Conversation

eshitachandwani commented Sep 3, 2024 • edited Loading

purnesh42H commented Sep 9, 2024

Choose a reason for hiding this comment

purnesh42H Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshitachandwani commented Sep 3, 2024 •

edited

Loading

purnesh42H Sep 9, 2024 •

edited

Loading