Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

otelgrpc: goleaks if context canceled #839

Closed
glazychev-art opened this issue Jun 30, 2021 · 1 comment
Closed

otelgrpc: goleaks if context canceled #839

glazychev-art opened this issue Jun 30, 2021 · 1 comment
Milestone

Comments

@glazychev-art
Copy link
Contributor

Hello,
We found that StreamClientInterceptor doesn't process context cancellation. And we see goleaks in this case.
For example, let's take a look at the next test:

func TestStreamClientInterceptorCancelContext(t *testing.T) {
	// enable goroutine leak detector 
	defer goleak.VerifyNone(t)

	clientConn, err := grpc.Dial("fake:connection", grpc.WithInsecure())
	if err != nil {
		t.Fatalf("failed to create client connection: %v", err)
	}
	defer clientConn.Close()

	// tracer
	sr := NewSpanRecorder()
	tp := oteltest.NewTracerProvider(oteltest.WithSpanRecorder(sr))
	streamCI := StreamClientInterceptor(WithTracerProvider(tp))

	var mockClStr mockClientStream
	method := "/github.com.serviceName/bar"
	name := "github.com.serviceName/bar"
	
	// create a context with cancel
	cancelCtx, cancel := context.WithCancel(context.Background())
	defer cancel()
	streamClient, err := streamCI(
		cancelCtx,
		&grpc.StreamDesc{ServerStreams: true},
		clientConn,
		method,
		func(ctx context.Context,
			desc *grpc.StreamDesc,
			cc *grpc.ClientConn,
			method string,
			opts ...grpc.CallOption) (grpc.ClientStream, error) {
			mockClStr = mockClientStream{Desc: desc, Ctx: ctx}
			return mockClStr, nil
		},
	)
	require.NoError(t, err, "initialize grpc stream client")
	_, ok := getSpanFromRecorder(sr, name)
	require.False(t, ok, "span should ended while stream is open")

	req := &mockProtoMessage{}
	reply := &mockProtoMessage{}

	// send and receive fake data
	for i := 0; i < 10; i++ {
		_ = streamClient.SendMsg(req)
		_ = streamClient.RecvMsg(reply)
	}

	// close client stream
	_ = streamClient.CloseSend()
}

Output:

=== RUN   TestStreamClientInterceptorCancelContext
    leaks.go:78: found unexpected goroutines:
        [Goroutine 9 in state chan receive, with go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.wrapClientStream.func1 on top of the stack:
        goroutine 9 [chan receive]:
        go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.wrapClientStream.func1(0xc00002cae0, 0xc00002ca80, 0xc00002cb40)
        	/home/art/xor/opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go:214 +0x78
        created by go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.wrapClientStream
        	/home/art/xor/opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go:208 +0xc5
        
         Goroutine 10 in state chan receive, with go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.StreamClientInterceptor.func1.1 on top of the stack:
        goroutine 10 [chan receive]:
        go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.StreamClientInterceptor.func1.1(0xc000021d40, 0xa58838, 0xc00037c240)
        	/home/art/xor/opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go:290 +0x59
        created by go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.StreamClientInterceptor.func1
        	/home/art/xor/opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go:289 +0x8ad
        ]
--- FAIL: TestStreamClientInterceptorCancelContext (0.44s)
FAIL

But, as we can see here:
https://github.com/grpc/grpc-go/blob/master/stream.go#L141
context cancellation is a valid situation and there should be no goleaks.

What do you think about this?
Thank you

@MrAlias
Copy link
Contributor

MrAlias commented Jul 21, 2021

Resolved in #840

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants