otelgrpc: goroutine leak if Recv is not called

[glazychev-art]

Hello,
This problem is related to StreamClientInterceptor.
The goroutine is creating inside wrapClientStream and it is waiting for events (ex. from RecvMsg)
https://github.com/open-telemetry/opentelemetry-go-contrib/blob/master/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L279-L280

What if we create clientStream and never call stream.Recv()? Seems there will be a goleak, because goroutine is waiting for events to quit.

Thank you

[XSAM]

clientStream considers grpc.ClientStream.CloseSend() as a kind of events. So after grpc.ClientStream.CloseSend() is called, the goroutine that is waiting for events will correctly exit.

opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go

Lines 186 to 196 in e532370

	func (w *clientStream) CloseSend() error {
	err := w.ClientStream.CloseSend()
	if err != nil {
	w.sendStreamEvent(errorEvent, err)
	} else {
	w.sendStreamEvent(closeEvent, nil)
	}
	return err
	}

It seems no goroutine will leak because of no stream.Recv() be called.

[glazychev-art]

Yes, but if we look at this:

opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go

Lines 214 to 229 in e532370

	for event := range events {
	switch event.Type {
	case closeEvent:
	state |= clientClosedState
	case receiveEndEvent:
	state |= receiveEndedState
	case errorEvent:
	finished <- event.Err
	return
	}
	if state == clientClosedState|receiveEndedState {
	finished <- nil
	return
	}
	}

we can see that return will happen if we get errorEvent OR (closeEvent AND receiveEndEvent).
You are right, we can get CloseEvent from CloseSend(), but ReceiveEndEvent will never happen if we don't call RecvMsg.
Maybe the condition should be like this:

			if (state & clientClosedState) != 0 || (state & receiveEndedState) != 0  {
				finished <- nil
				return
			}

Correct me if I am wrong

[XSAM]

@glazychev-art You're right that goroutine will leak if we don't call RecvMsg. But it is not a complete gRPC call if we don't call RecvMsg.

Also, CloseSend only means closing the send direction of the stream, not the entire stream, which means the stream is still on, and we can't end the span at this time.

[denis-tingaikin]

@XSAM Is it possible to wait for <-stream.Context().Done() and close all resources when it happens? I guess it could solve the problem with

if we don't call RecvMsg. But it is not a complete gRPC call if we don't call RecvMsg.

[XSAM]

Is it possible to wait for <-stream.Context().Done() and close all resources when it happens?

@denis-tingaikin It might not be a solution here since the context can't be called before RecvMsg

Context() should not be called until after Header or RecvMsg has returned.

https://github.com/grpc/grpc-go/blob/577eb696279ea85069a02c9a4c2defafdab858c5/stream.go#L93-L97

[denis-tingaikin]

This issue still actual.

Moreover, it is not reproducible if we are using jaeger directly.

[XSAM]

@denis-tingaikin I can't reproduce this bug.

What version of the otelgrpc you are using? Also, what's the scenario that users might not call RecvMsg.

[glazychev-art]

It seems it has been fixed - #840
Feel free to reopen