Visitar URL original
[fix][client] Shut down admin TLS factory executor on resolution failure by HimanshuJha-2005 · Pull Request #27509 · apache/pulsar · GitHub
Skip to content

[fix][client] Shut down admin TLS factory executor on resolution failure - #27509

Open
HimanshuJha-2005 wants to merge 1 commit into
apache:masterfrom
HimanshuJha-2005:fix-26427-admin-tls-factory-thread-leak
Open

[fix][client] Shut down admin TLS factory executor on resolution failure#27509
HimanshuJha-2005 wants to merge 1 commit into
apache:masterfrom
HimanshuJha-2005:fix-26427-admin-tls-factory-thread-leak

Conversation

@HimanshuJha-2005

@HimanshuJha-2005 HimanshuJha-2005 commented Nov 9, 2026

Copy link
Copy Markdown

Fixes #26427

Motivation

In AsyncHttpConnector.resolveNewTlsFactory, a single-thread scheduled executor is created with DefaultThreadFactory("pulsar-admin-tls-factory"):

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);
return factory;

When AsyncHttpConnector is created via its public constructors (which supply TlsFactoryOwnership.none()), if ClientTlsFactorySupport.resolveClientTlsFactory throws an exception (e.g. invalid TLS parameters, unresolvable custom TLS factory, or failing factory initialization), this.tlsFactoryOwnership is never assigned. Consequently, the constructor's cleanup path (releasePartialResources) cannot close the executor.

Because Netty's DefaultThreadFactory creates non-daemon threads by default, the allocated pulsar-admin-tls-factory thread leaks and prevents the JVM from exiting.

AsyncHttpConnectorProvider.sharedTlsFactory() already guards the identical allocation with a try-catch block shutting down the executor on failure.

Modifications

  • In AsyncHttpConnector.resolveNewTlsFactory, wrap ClientTlsFactorySupport.resolveClientTlsFactory in a try ... catch (Throwable t) block and invoke executor.shutdownNow() before rethrowing.
  • Added unit test testResolveNewTlsFactoryShutdownExecutorOnFailure to AsyncHttpConnectorTest verifying that when TLS factory resolution fails, the allocated executor thread is promptly terminated.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added testResolveNewTlsFactoryShutdownExecutorOnFailure in AsyncHttpConnectorTest, verifying the thread leak is eliminated when factory resolution throws.
  • Verified :pulsar-client-admin-original:spotlessCheck passes cleanly.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

codelipenghui
codelipenghui requested changes Nov 9, 2026
View reviewed changes

@codelipenghui codelipenghui left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The runtime fix is correct and minimal. When ClientTlsFactorySupport.resolveClientTlsFactory throws, resolveNewTlsFactory now shuts down its private pulsar-admin-tls-factory executor. That matches AsyncHttpConnectorProvider.sharedTlsFactory() and the PIP-478 ownership split: the resolver closes the factory it initialized, the creator stops the executor, and ownership stays none(), so nothing is closed twice. The success path and the exception types callers see are unchanged.

I verified this before and after. With the main hunk reverted, the new test fails because thread pulsar-admin-tls-factory-2-1 is still alive. With the PR code it passes, and AsyncHttpConnectorTest (13 tests) and AdminTlsFactoryLifecycleTest (9 tests) also pass.

One blocker: the new test's imports break checkstyle's ImportOrder rule. CI runs checkstyleTest, so the PR fails CI as-is. Fix the import order (and switch the two inline fully qualified names to imports while there) and the change is ready.

Comment thread ...admin/src/test/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnectorTest.java Outdated
Comment on lines +49 to +53
import java.util.Optional;
import org.apache.pulsar.tls.PulsarTlsFactory;
import org.apache.pulsar.tls.TlsFactoryInitContext;
import org.apache.pulsar.tls.TlsHandle;
import org.apache.pulsar.tls.TlsPurpose;

@codelipenghui codelipenghui Nov 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

New test imports break checkstyle ImportOrder, which CI enforces via checkstyleTest

Any CI run, or locally './gradlew :pulsar-client-admin-original:checkstyleTest'. The "Build, check licenses and code style" step in .github/workflows/pulsar-ci.yaml:196 runs './gradlew assemble rat spotlessCheck checkstyleMain checkstyleTest'. A required CI step fails every time, so the PR cannot merge as-is. The description says CI passes, but only spotlessCheck was run locally, and spotless does not check import order.

Ran ':pulsar-client-admin-original:checkstyleTest' at head 84748f5. The task FAILED with two errors:

  • 'AsyncHttpConnectorTest.java:49:1: Wrong lexicographical order for 'java.util.Optional' import. Should be before 'java.util.concurrent.atomic.AtomicReference'. [ImportOrder]'
  • 'AsyncHttpConnectorTest.java:54:1: Wrong lexicographical order for 'lombok.Cleanup' import. Should be before 'org.apache.pulsar.tls.TlsPurpose'. [ImportOrder]'

buildtools/src/main/resources/pulsar/checkstyle.xml:124-132 sets ImportOrder to error severity, with non-static imports in one ASCII-sorted group. ':pulsar-client-admin-original:checkstyleMain' passes, so only the test file is affected.

Put 'import java.util.Optional;' right after 'import java.util.List;'. Move the four 'org.apache.pulsar.tls.' imports after 'import org.apache.pulsar.common.util.FutureUtil;', so 'lombok.Cleanup' comes before all 'org.' imports. While editing the imports, add 'import java.util.concurrent.CountDownLatch;' (after 'CopyOnWriteArrayList') and 'import java.util.function.Consumer;' (after 'atomic.AtomicReference'), and use the simple names at lines 545 and 569 instead of the inline fully qualified names, per the AGENTS.md style rule. Then re-run './gradlew :pulsar-client-admin-original:checkstyleTest'.

@HimanshuJha-2005
[fix][client] Shut down admin TLS factory executor on resolution failure
3e8dd43 
Fixes apache#26427

In AsyncHttpConnector.resolveNewTlsFactory, a single-thread scheduled executor is allocated for the pulsar-admin-tls-factory thread. When ClientTlsFactorySupport.resolveClientTlsFactory throws an exception during resolution or initialization, the executor is never shut down. Because Netty's DefaultThreadFactory creates non-daemon threads by default, this leaks the thread and prevents JVM termination.

Wrap the factory resolution in a try-catch block to ensure executor.shutdownNow() is called if resolution throws, mirroring the error handling in AsyncHttpConnectorProvider.
@HimanshuJha-2005
HimanshuJha-2005 force-pushed the fix-26427-admin-tls-factory-thread-leak branch from 84748f5 to 3e8dd43 Compare November 9, 2026 18:44
@HimanshuJha-2005

HimanshuJha-2005 commented Nov 9, 2026

Copy link
Copy Markdown
Author

Thanks for the review @codelipenghui! Fixed the import order to satisfy checkstyle and replaced the inline fully qualified names with imports. Re-verified that :pulsar-client-admin-original:checkstyleTest and spotlessCheck both pass cleanly.

@lhotari

lhotari commented Nov 10, 2026

Copy link
Copy Markdown
Member

Thanks for the review @codelipenghui! Fixed the import order to satisfy checkstyle and replaced the inline fully qualified names with imports. Re-verified that :pulsar-client-admin-original:checkstyleTest and spotlessCheck both pass cleanly.

Thanks for the PR.

building reference with quickCheck and sanityCheck tasks description:

the shortcut for quickCheck is qC so you can execute ./gradlew qC to ensure that checkstyle passes before pushing.

To run full CI pipeline in your own fork, please read Personal CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@HimanshuJha-2005 @lhotari @codelipenghui