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

[extension/jaegarremotesamplingextension/internal] Leaking goroutine on shutdown #31157

Closed
crobert-1 opened this issue Feb 9, 2024 · 4 comments · Fixed by #31661
Closed

[extension/jaegarremotesamplingextension/internal] Leaking goroutine on shutdown #31157

crobert-1 opened this issue Feb 9, 2024 · 4 comments · Fixed by #31661
Assignees
Labels

Comments

@crobert-1
Copy link
Member

Component(s)

extension/jaegerremotesampling

Describe the issue you're reporting

When attempting to add goleak to the internal package of the jaegarremotesampling extension, the following test fails:

=== RUN   TestSamplingGRPCServer_Shutdown
--- PASS: TestSamplingGRPCServer_Shutdown (10.00s)
=== RUN   TestSamplingGRPCServer_Shutdown/graceful_stop_is_successful_without_delay
    --- PASS: TestSamplingGRPCServer_Shutdown/graceful_stop_is_successful_without_delay (0.00s)
=== RUN   TestSamplingGRPCServer_Shutdown/graceful_stop_is_successful_with_delay
    --- PASS: TestSamplingGRPCServer_Shutdown/graceful_stop_is_successful_with_delay (5.00s)
=== RUN   TestSamplingGRPCServer_Shutdown/context_timed_out
    --- PASS: TestSamplingGRPCServer_Shutdown/context_timed_out (5.00s)
=== RUN   TestSamplingGRPCServer_Shutdown/grpc_server_not_started
    --- PASS: TestSamplingGRPCServer_Shutdown/grpc_server_not_started (0.00s)
PASS
goleak: Errors on successful test run: found unexpected goroutines:
[Goroutine 35 in state sleep, with time.Sleep on top of the stack:
time.Sleep(0xdf8475800)
	/usr/local/go/src/runtime/time.go:195 +0x125
github.com/open-telemetry/opentelemetry-collector-contrib/extension/jaegerremotesampling/internal.(*grpcServerMock).GracefulStop(0x0?)
	/Users/crobert/dev/opentelemetry-collector-contrib/extension/jaegerremotesampling/internal/grpc_test.go:91 +0x16
github.com/open-telemetry/opentelemetry-collector-contrib/extension/jaegerremotesampling/internal.(*SamplingGRPCServer).Shutdown.func1()
	/Users/crobert/dev/opentelemetry-collector-contrib/extension/jaegerremotesampling/internal/grpc.go:95 +0x2f
created by github.com/open-telemetry/opentelemetry-collector-contrib/extension/jaegerremotesampling/internal.(*SamplingGRPCServer).Shutdown in goroutine 34
	/Users/crobert/dev/opentelemetry-collector-contrib/extension/jaegerremotesampling/internal/grpc.go:94 +0xa5
]

The actual failing test is TestSamplingGRPCServer_Shutdown/context_timed_out.

The leaked goroutine is started here:

What's happening is the Shutdown method for the sampling GRPC server starts a goroutine that attempts to gracefully shutdown the server. As soon as the context is cancelled though (graceful stop took too long), Stop is called and the main routine will return, leaking the sub-goroutine that's attempting GracefulStop.

@crobert-1 crobert-1 added needs triage New item requiring triage bug Something isn't working labels Feb 9, 2024
Copy link
Contributor

github-actions bot commented Feb 9, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@yurishkuro
Copy link
Member

@crobert-1 are you addressing this yourself or looking for help?

@crobert-1
Copy link
Member Author

Looking for help at the moment. If no one's able to take this soon I'll plan on eventually proposing a solution. 👍

@chirag-ghosh
Copy link

I would like to take this up

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Feb 26, 2024
dmitryax pushed a commit that referenced this issue Mar 27, 2024
…or jaegerremotesampling extension (#31661)

Related to
#30438
Resolves #31157

Signed-off-by: Israel Blancas <iblancasa@gmail.com>
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this issue May 8, 2024
…or jaegerremotesampling extension (open-telemetry#31661)

Related to
open-telemetry#30438
Resolves open-telemetry#31157

Signed-off-by: Israel Blancas <iblancasa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants