apache / apache/pulsar

[fix][misc] PIP-478: lock inversion between a custom PulsarTlsFactory reload callback and TlsContextAcquisition.dispose()

Open
#26,387 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
15.3k
Forks
3.8k
Avg merge
1d 14h
Merged PRs (30d)
160

Description

### Motivation

`TlsContextAcquisition.SynthesizingSubscription` takes its own monitor and, while holding it, calls back
into the factory:

```java
@Override
public synchronized void dispose() {
// Synchronized on the same monitor as publish(), so this returns only once any in-flight publish
// has finished ... The callback contract is a cheap non-blocking store, so waiting here is bounded.
disposed.set(true);
TlsHandle jdkHandle = underlying;
if (jdkHandle != null) {
jdkHandle.dispose(); // <- delegate dispose INSIDE the monitor
}
}
```

`publish(...)` is `synchronized` on that same monitor, and `onDelivery` reaches it inline on the
factory's delivery thread:

```java
factory.createInstance(purpose, SSLParameters.class).whenComplete((companion, err) ->
publish(jdkContext, err != null ? null : extractBaseline(companion), generation, err == null));
```

For a factory that answers `Optional.empty()` for the `SSLParameters` companion — the obvious
implementation for a factory with no companion — that future is already complete, so `whenComplete` runs
on the calling thread.

The reference lock discipline is in the same module: `FileBasedTlsFactory.deliverToSubscribers` is
`synchronized` on the per-purpose source and invokes the consumer callback while holding it, and
`removeSubscription` — reached from `SubscriptionHandle.dispose()` — takes that same monitor.

So with a custom factory shaped like the built-in one:

- **Thread A** (rotation poll): holds the factory's source monitor → runs the reload callback →
`onDelivery` → `whenComplete` inline → `publish` → blocks on the subscription monitor.
- **Thread B** (consumer shutdown): holds the subscription monitor in `dispose()` → `jdkHandle.dispose()`
→ blocks on the factory's source monitor.

Both hang permanently: the component hangs on close and the TLS rotation poll thread is lost for the rest
of the process's life.

### Scope

Not reachable with the built-in factory. `TlsContextAcquisition.acquireNettyContext` asks for
`SslContext.class` first and `FileBasedTlsFactory.isSupported` covers it, so `direct.isPresent()` and the
synthesis path is skipped entirely. It needs a **custom** `PulsarTlsFactory` that supplies only
`SSLContext` — which is exactly the minimal implementation the SPI advertises ("as long as it supplies at
least the `SSLContext`, the framework can derive the Netty and Jetty objects"), and the SPI's
"reload callbacks are serial per subscription" requirement pushes an implementer toward the same
per-purpose lock that deadlocks.

The `dispose()` comment argues the wait is bounded because the consumer callback is a cheap store. That
reasoning covers a `publish` already running; it does not cover a `publish` blocked on the monitor while
holding the factory's lock.

### Suggested fix

Move `jdkHandle.dispose()` outside the synchronized block (set and read state under the monitor, then
call the delegate unlocked), or drop the monitor from `dispose()` and rely on the `disposed` flag plus
the generation guard already in `publish`. The framework should never call back into a factory while
holding a framework lock that a factory-invoked callback also needs.

Worth stating the resulting rule in the `PulsarTlsFactory` javadoc as well, since a factory author
currently has no way to know which locks the framework may hold when it calls them.

---

Found in a final-state review of the PIP-478 series (#26321); mechanics verified by reading the lock
acquisition order, not by reproducing the hang.

Contributor guide

Open the contributing guide

Research direction

Start with TlsContextAcquisition.SynthesizingSubscription.dispose and publish, then compare their lock ordering with FileBasedTlsFactory.deliverToSubscribers and removeSubscription. Review the PulsarTlsFactory reload-callback contract and javadoc. Done means the custom SSLContext-only factory path cannot deadlock during disposal, with coverage or verification for the lock inversion and the callback-locking rule documented.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.