Skip to content

Shutting down a grpcsender without interrupting the thread #7557

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Shutting down a grpcsender without interrupting the thread #7557

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...http/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ private static String grpcMessage(Response response) {
public CompletableResultCode shutdown() {
client.dispatcher().cancelAll();
if (managedExecutor) {
client.dispatcher().executorService().shutdownNow();
client.dispatcher().executorService().shutdown();
Copy link
Member

Choose a reason for hiding this comment

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

do you know if the executor is using daemon threads, or do we need to worry about this possibly impacting application shutdown?

Copy link

@bidetofevil bidetofevil Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So managedExecutor is only true if no ExecutorService is NOT passed into this class, which means we are purging the ExecutorService only when we are using OktHttp default ExecutorService for the Dispatcher.

Shouldn't it be the other way around, i.e. we only explicitly shut down the executor if we pass it in ourselves, and leave the clean up to OkHttp if it's providing its own executor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So managedExecutor is only true if no ExecutorService is passed into this class, which means we are purging the ExecutorService only when we are using OktHttp default ExecutorService for the Dispatcher.

I think we never use okhttp's default service, as we set one here when users don't provide any.

do you know if the executor is using daemon threads, or do we need to worry about this possibly impacting application shutdown?

We are using daemon threads, but it's not guaranteed that it will always be the case. I didn't think of the app not closing due to a non-daemon thread scenario, because Android will still close an app regardless of existing non-daemon threads running, but it's a great point. Also, @laurit mentioned below that we're using shutdownNow in many places, so this doesn't seem to be a proper fix overall. I'll work on an alternative.

Copy link

@bidetofevil bidetofevil Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

LOL, sorry, I flipped the logic yesterday. What I meant to say is that managedExecutor is only true if you DON'T pass in an executor, so this codepath should never be hit if we always passed one in.

THAT was the logic that I thought seems a bit off.

That being said, handing the uncaught exception explicitly seems like a good idea. Android won't crash the app until the uncaught exception is thrown on the main thread, but silently crashing a background thread isn't great either if you can deal with it explicitly.

}
client.connectionPool().evictAll();
return CompletableResultCode.ofSuccess();
Expand Down
Loading