[Bug] AsyncHttpConnector.resolveNewTlsFactory leaks the pulsar-admin-tls-factory thread when TLS factory resolution throws
- Dominant language
- Java
- Stars
- 15.3k
- Forks
- 3.8k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 160
Description
**Search before asking**
- [X] I searched in the [issues](https://github.com/apache/pulsar/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:
```java
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:
```java
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?
- [X] I'm willing to submit a PR!
Contributor guide
Assessment
This issue has not been assessed yet.