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

gRPC: fix goroutine leak if client stream CloseSend is not called. #836

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

vadimberezniker
Copy link
Contributor

@vadimberezniker vadimberezniker commented Jun 29, 2021

Outside of a unidirectional Client->Server stream there's no requirement
that CloseSend must be called (and it doesn't make much sense for a
Server->Client stream).

If CloseSend is not called, the RPC span will not be ended and a
goroutine will be leaked.

We saw a large number of goroutines leaked when we enabled gRPC
instrumentation in our service.

Resolves #818

Outside of a unidirectional Client->Server stream there's no requirement
that CloseSend must be called (and it doesn't make much sense for a
Server->Client stream).

If CloseSend is not called, the RPC span will not be ended and a
goroutine will be leaked.

We saw a large number of goroutines leaked when we enabled gRPC
instrumentation in our service.

Someone else has also reported this issue here:
open-telemetry#818
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 29, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #836 (5185fff) into main (e8b0fa6) will decrease coverage by 0.3%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #836     +/-   ##
=======================================
- Coverage   79.4%   79.1%   -0.4%     
=======================================
  Files         62      62             
  Lines       2755    2748      -7     
=======================================
- Hits        2188    2174     -14     
- Misses       437     443      +6     
- Partials     130     131      +1     
Impacted Files Coverage Δ
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 86.1% <100.0%> (-3.2%) ⬇️

@vadimberezniker
Copy link
Contributor Author

Friendly ping :)

@vadimberezniker
Copy link
Contributor Author

Ping. This PR has been open for 2 weeks now.

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@vadimberezniker
Copy link
Contributor Author

Thank you! Any blockers for merging this?

@vadimberezniker
Copy link
Contributor Author

What's left?

@MrAlias MrAlias merged commit 2b2b424 into open-telemetry:main Jul 21, 2021
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

Successfully merging this pull request may close these issues.

otelgprc: stream not closed on eof
6 participants