[Bug] AsyncHttpConnector.resolveNewTlsFactory leaks the pulsar-admin-tls-factory thread when TLS factory resolution throws

[david-streamlio]

Search before asking

• I searched in the issues and found nothing similar.

Version

master (ecf6daeac9).

Minimal reproduce step

Construct an AsyncHttpConnector through one of its two public constructors — the ones taking TlsFactoryOwnership.none() — with a client configuration that needs a TLS factory but is invalid enough for ClientTlsFactorySupport.resolveClientTlsFactory to throw (for example a tlsTrustCertsFilePath that does not exist, or a by-name custom factory whose initialize() fails).

What did you expect to see?

The pulsar-admin-tls-factory thread shut down as the exception propagates, leaving no live thread behind.

What did you see instead?

The thread survives the failed construction. Because Netty's DefaultThreadFactory creates non-daemon threads by default, it can keep the JVM from exiting.

Anything else?

AsyncHttpConnector.resolveNewTlsFactory creates the executor and then calls resolveClientTlsFactory with no try, so nothing shuts it down if that call throws:

ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(
        new DefaultThreadFactory("pulsar-admin-tls-factory"));
PulsarTlsFactory factory = ClientTlsFactorySupport.resolveClientTlsFactory(conf, executor,
        executor, conf.getOpenTelemetry());
this.tlsFactoryOwnership = TlsFactoryOwnership.owning(factory, executor);

AsyncHttpConnectorProvider.sharedTlsFactory() does the identical thing and guards it:

ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(
        new DefaultThreadFactory("pulsar-admin-tls-factory"));
try {
    sharedTlsFactory = TlsFactoryOwnership.owning(
            ClientTlsFactorySupport.resolveClientTlsFactory(conf, executor, executor,
                    conf.getOpenTelemetry()),
            executor);
} catch (Exception e) {
    executor.shutdownNow();
    throw ...;
}

So the connector is the odd one out of two sites that create the same named executor for the same purpose. PulsarAdminImpl's constructed = false path names that same thread as the thing worth not leaking, which suggests the guard was simply missed here rather than deliberately omitted.

Reachability. In tree this is currently unreachable: AsyncHttpConnectorProvider is the only caller of the relevant constructor and always supplies an already-resolved factory, so suppliedTlsFactory.isPresent() short-circuits before the executor is created. It is reachable from out-of-tree code and from tests, because AsyncHttpConnector has two public constructors that pass TlsFactoryOwnership.none().

Suggested fix: wrap the resolveClientTlsFactory call in resolveNewTlsFactory the same way sharedTlsFactory() does, shutting the executor down before rethrowing, and add a test that a failed resolution leaves no pulsar-admin-tls-factory thread running.

Found while reviewing #26326; deliberately kept out of that PR since it is pre-existing and unrelated to its scope.

Are you willing to submit a PR?

• I'm willing to submit a PR!