[extension/jaegarremotesamplingextension/internal] fix leaking goroutine on shutdown #31278
Conversation
Signed-off-by: Chirag Ghosh <[email protected]>
Signed-off-by: Chirag Ghosh <[email protected]>
|
I had to make the Shutdown function wait for the goroutine to finish because there is no way to force stop it. The Stop() function should close all connections while still waiting for the GracefulStop() to finish execution. |
| @@ -99,6 +99,7 @@ func (s *SamplingGRPCServer) Shutdown(ctx context.Context) error { | |||
| select { | |||
| case <-ctx.Done(): | |||
| s.grpcServer.Stop() | |||
| <-ch // wait for the graceful stop goroutine to return | |||
There was a problem hiding this comment.
what is the pattern used by OTLP server in main collector repo? We should use the same mechanisms
There was a problem hiding this comment.
The only grpc implementation I found was in the otlp receiver here. It uses a waiting group which gets complete when the server is actually stopped after calling GracefulStop(). So, it does not take into account the context timeout as found in our code. Should I change the implementation to match this?
P.S. This is also how all the receivers using grpc are configured in the contrib repository (including jaeger-receiver)
There was a problem hiding this comment.
sgtm - if it works for the exporter which is most likely used in 100% of configurations, why does this one needs to be more elaborate for shutdown.
There was a problem hiding this comment.
Pushed the changes. Do we need to tweak the tests?
Signed-off-by: Chirag Ghosh <[email protected]>
|
I think it's okay to skip adding a changelog here as even though it's a bug fix, it's an internal package. |
Co-authored-by: Curtis Robert <[email protected]>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description: Fix goroutine leak on grpc server shutdown
Link to tracking Issue: #31157
Testing: Add goleak check for internal package. Run make test at module level.
Documentation: