otelgrpc: fix goroutine leak after context cancellation

[glazychev-art]

This PR is related to this issue: #839

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Artem Glazychev (2a635b9)

[glazychev-art]

@vadimberezniker @tonistiigi
Hello. As far as I know, you have caught a similar problem. Could you please take a look too?

[vadimberezniker]

Disclaimer: I'm not a maintainer on this project but I have been looking at this code recently.

If we trust the ClientStream.Context() docs then it's not safe to use the Context() call here:

	// Context returns the context for this stream.
	//
	// It should not be called until after Header or RecvMsg has returned. Once
	// called, subsequent client-side retries are disabled.
	Context() context.Context

Edit: I just double checked the gRPC code and Context() does have side-effects.

The alternative is to pass the parent context and check that.

Thanks for fixing this, I noticed this while fixing the other issue and this was next on my plate.

[tonistiigi]

Does <-s.Context().Done() return then calling context is canceled or when stream is closed from the server-side and recv return eof? If former then I don't think this is correct and we should wait for server-side to close stream, maybe calling recvmsg is goroutine is an option if there isn't anything better.

[vadimberezniker]

Does <-s.Context().Done() return then calling context is canceled or when stream is closed from the server-side and recv return eof? If former then I don't think this is correct and we should wait for server-side to close stream, maybe calling recvmsg is goroutine is an option if there isn't anything better.

If the client properly handles the stream, the ctx.Done() case should not be reached. That should be handled by the event for loop.
The issue @glazychev-art is trying to address is the case where the parent context is cancelled before the client is done with the stream.

[tonistiigi]

@vadimberezniker I understand that. The canceling flow should be 1) context client passed gets canceled 2) cancellation is passed to the server-side 3) server closes either cleanly or with an error 4) client receives the close and stream is finished. Just cancelling the context is just a signal and doesn't atomically close the stream by itself. I'm not sure about the internals, therefore it may be possible that s.Context() only closes after 4), in that case, the change looks correct to me. But if it closes after 1) then I don't think this approach is correct.

[glazychev-art]

@vadimberezniker
Thank you. Updated to parent context

[glazychev-art]

@tonistiigi
It is a good idea, but I'm not sure if this possible.. Because, let's take a look at:
https://github.com/grpc/grpc-go/blob/master/stream.go#L358-L372

		go func() {
			select {
			case <-cc.ctx.Done():
				cs.finish(ErrClientConnClosing)
			case <-ctx.Done():
				cs.finish(toRPCErr(ctx.Err()))
			}
		}()

ClientStream is keeping track context closing. And does not notify about it.
So, if we call RecvMsg it may be already finished.

[vadimberezniker]

@vadimberezniker I understand that. The canceling flow should be 1) context client passed gets canceled 2) cancellation is passed to the server-side 3) server closes either cleanly or with an error 4) client receives the close and stream is finished. Just cancelling the context is just a signal and doesn't atomically close the stream by itself. I'm not sure about the internals, therefore it may be possible that s.Context() only closes after 4), in that case, the change looks correct to me. But if it closes after 1) then I don't think this approach is correct.

Based on my understanding when the context is cancelled, gRPC automatically closes the underlying HTTP2 stream so there's no opportunity for the server to send anything after that.

[tonistiigi]

I did some tests and indeed the server-side graceful cancel does not happen as I described before. I had forgotten that in the code where I use it I actually need to do two requests and only cancel one of them (that forces another to close cleanly) to achieve this behavior.

But it is still possible that RecvMsg returns cleanly after the context has been canceled if server-side sent the message before it received the cancel(after that sends will fail). Looking at the interceptor I think it probably just means that the events for the receive get dropped that isn't catastrophic. Although, I think I still kind of consider it a caller side issue if the client just cancels context and doesn't recv until an error after that.

[glazychev-art]

@Aneurysm9 @dashpole @evantorrie @jmacd @MadVikingGod @MrAlias @paivagustavo @XSAM
Any thoughts on this?

[Aneurysm9]

The approach seems sane. Please fix the linting issue.

[glazychev-art]

@Aneurysm9
Thank you. Fixed

[codecov]

Codecov Report

Merging #840 (da5c1aa) into main (e2210a9) will increase coverage by 0.2%.
The diff coverage is 81.2%.

@@           Coverage Diff           @@
##            main    #840     +/-   ##
=======================================
+ Coverage   79.2%   79.4%   +0.2%     
=======================================
  Files         62      62             
  Lines       2750    2755      +5     
=======================================
+ Hits        2178    2188     +10     
+ Misses       440     437      -3     
+ Partials     132     130      -2     

Impacted Files	Coverage Δ
...ion/google.golang.org/grpc/otelgrpc/interceptor.go	89.2% <81.2%> (+2.2%)	⬆️

[XSAM]

Looks good overall.

[edwarnicke]

@pellared Pardon my ignorance of the process... but what else has to be done before this fix can be merged?

[pellared]

@Aneurysm9 @MrAlias PTAL

[edwarnicke]

Yay!