apache / apache/pulsar

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

Open
#26,427 0 comments 0 reactions 1 assignee Claimed by @lhotari View on GitHub
type/bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.