transport : wait for goroutines to exit before transport closes

[eshitachandwani]

Fixes: #2869

Previously, clientconn.Close() did not guarantee termination of some goroutines upon return.

Now, clientconn.Close() ensures that:

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

RELEASE NOTES:

• transport : fixed a bug where the transport was closed before all goroutines had exited.
• grpc : transports are closed simultaneously, and clientconn.Close() waits for all transports to be closed before returning.

[codecov]

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.93%. Comparing base (859602c) to head (a09b7aa).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/transport/http2_client.go	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7666      +/-   ##
==========================================
+ Coverage   81.85%   81.93%   +0.08%     
==========================================
  Files         361      361              
  Lines       27822    27845      +23     
==========================================
+ Hits        22773    22815      +42     
+ Misses       3851     3837      -14     
+ Partials     1198     1193       -5     

Files with missing lines	Coverage Δ
clientconn.go	92.65% <100.00%> (+0.05%)	⬆️
internal/transport/http2_client.go	92.21% <91.30%> (+0.58%)	⬆️

... and 34 files with indirect coverage changes

[aranjans]

Thanks for addressing my earlier comments, overall change LGTM but still I feel it'd be better to add e2e style tests to verify the same.

[eshitachandwani]

Thanks for addressing my earlier comments, overall change LGTM but still I feel it'd be better to add e2e style tests to verify the same.

Yes, I am working on the tests for it.

[eshitachandwani]

We rely on leak check to detect if keepAlive goroutine does not exit before transport closes as it is difficult to determine when keepAlive function exits to write a test for it.

[easwars]

Looks good in general. Mostly nits.

[easwars]

Suggested change

	// Tests that client does not close until the reader goroutine exits and closes
	// once reader goroutine returns.
	// Tests that closing a client transport does not return until the reader goroutine exits.

[eshitachandwani]

Done.

[easwars]

Nit: s/connectCtx/ctx

[eshitachandwani]

Done.

[easwars]

Nit: why not use ctx.Done() here? That will ensure that the whole test has only 10s to run.

[eshitachandwani]

This change is somehow causing a race condition between TestClientCloseReturnsEarlyWhenGoAwayWriteHangs and TestClientCloseReturnsAfterReaderCompletes

[easwars]

Hmm... That is weird. Tests should be independent. Do you have an output from a failed run?

[easwars]

Nit: single line return hc.Conn.Close()