`JdkHttpSender` leaks `SelectorManager` thread

[felixbarny]

JdkHttpSender creates a java.net.http.HttpClient, which in turn creates an internal jdk.internal.net.http.HttpClientImpl.SelectorManager thread. When calling JdkHttpSender#shutdown, it doesn't terminate that thread.

The issue is that java.net.http.HttpClient#close has only been introduced in Java 21.

@jack-berg what do you think about adding a reflection-based check and call close when it's available?

I was thinking something like this:

    if (AutoCloseable.class.isInstance(client)) {
      try {
        AutoCloseable.class.cast(client).close();
      } catch (Exception e) {
        return CompletableResultCode.ofExceptionalFailure(e);
      }
    }

In JdkHttpSender#shutdown

opentelemetry-java/exporters/sender/jdk/src/main/java/io/opentelemetry/exporter/sender/jdk/internal/JdkHttpSender.java

Lines 388 to 394 in a756317

	@Override
	public CompletableResultCode shutdown() {
	if (managedExecutor) {
	executorService.shutdown();
	}
	return CompletableResultCode.ofSuccess();
	}

WDYT?

[felixbarny]

I've created a PR for this: #7390

[RussianPrussian]

Can we fix for earlier versions of Java as well? The right way to do that would be to build the http client with a passed in thread pool and to shut it down after.

[jack-berg]

Can we fix for earlier versions of Java as well? The right way to do that would be to build the http client with a passed in thread pool and to shut it down after.

This should already be the case...

• If you don't explicitly call setExecutorService, we initialize one on your behalf, and manage its lifecycle by shutting it down when needed
• You might notice that we don't call HttpClient.Builder#setExecutor when building our HttpClient instance. Instead, we:
  • Run the entire send workflow using the resolved ExecutorService: https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/sender/jdk/src/main/java/io/opentelemetry/exporter/sender/jdk/internal/JdkHttpSender.java#L178
  • Only ever call HttpClient#send (i.e. no sendAsync) when executing requests
  • The net effect should be that only the user-configured or JdkHttpSender managed ExecutorService should be used to execute requests. Although, when I read the javadoc carefully, I see references to "dependent tasks" also getting completed by HttpClient's executors. Not sure what this refers, but if HttpClient is initializing an ExecutorService and executing work on an executor other than the one we're managing, then we would want to call HttpClient.Builder#executor. Although when I tried this, I got test failures if my configured thread pool was smaller than two threads.

[felixbarny]

The issue is that even if you provide an executor, a SelectorManager thread gets instantiated in the HttpClientImpl constructor. There seems to be no way to close it (other than maybe interrupting it) before the java.net.http.HttpClient#close method has only been introduced in Java 21.

[jack-berg]

The issue is that even if you provide an executor, a SelectorManager thread gets instantiated in the HttpClientImpl constructor.

😞