[fix][misc] PIP-478: lock inversion between a custom PulsarTlsFactory reload callback and TlsContextAcquisition.dispose()
- 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
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