Skip to content

Commit 1e83695

Browse files
committed
[cleanup][client] PIP-478: say what the self-suppression guard actually is
The comment justified the identity guard in closeQuietly as "Identity-guarded, as try-with-resources generates". It is not. The JLS 14.20.3.1 translation calls addSuppressed unguarded, javac's lowering emits no identity test, and a self-suppressing resource in a try-with-resources block propagates the same IllegalArgumentException this guard exists to prevent — confirmed by running one (it prints "PROPAGATED: java.lang.IllegalArgumentException: Self-suppression not permitted") and by javap: 20: invokevirtual #19 // Method R.close:()V 26: astore_2 27: aload_1 28: aload_2 29: invokevirtual #22 // Method java/lang/Throwable.addSuppressed no comparison between the two throwables. That matters because the comment is load-bearing: it is there to stop someone deleting the guard as redundant, and it justified it by a precedent that would not have caught the bug. The guard is STRICTER than try-with-resources, which is both true and the useful thing to say. Two more corrections in the same comment, from cross-review: - "The probe path could not do this — it always mints a fresh one" was false. probe()'s own throws are all freshly minted, but it calls TlsHandle.dispose() outside any wrapping catch, so an unchecked throwable from dispose() propagates by identity and close() could rethrow that same instance. The probe path is a second route to self-suppression, not an impossibility. - The javadoc promised to attach "any close failure". It attaches any DISTINCT one; the identical case is precisely what the guard drops. Also records that addSuppressed validates its argument even when suppression is disabled, so the guard is needed regardless of how the propagating throwable was constructed. Comment-only; no behaviour change. The guard itself and aFactoryThatFailsInitializeAndCloseWithOneExceptionKeepsTheRealFailure are untouched.
1 parent c07157a commit 1e83695

1 file changed

Lines changed: 16 additions & 9 deletions

File tree

pulsar-client/src/main/java/org/apache/pulsar/client/impl/tls/ClientTlsFactorySupport.java

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -645,8 +645,9 @@ private static void initializeBlocking(PulsarTlsFactory factory, TlsFactoryInitC
645645
}
646646

647647
/**
648-
* Close a factory the framework initialized, or tried to, attaching any close failure to the original
649-
* error so the cleanup problem is visible without displacing the reason the build failed.
648+
* Close a factory the framework initialized, or tried to, attaching any <em>distinct</em> close failure
649+
* to the original error so the cleanup problem is visible without displacing the reason the build failed.
650+
* A close failure that is the original error itself is deliberately not attached — see below.
650651
*
651652
* @param factory the factory to release — initialized, or left half-built by a failed {@code initialize}
652653
* @param failure the failure being propagated
@@ -655,13 +656,19 @@ private static void closeQuietly(PulsarTlsFactory factory, Throwable failure) {
655656
try {
656657
factory.close();
657658
} catch (Throwable closeFailure) {
658-
// Identity-guarded, as try-with-resources generates: addSuppressed throws
659-
// IllegalArgumentException("Self-suppression not permitted") when handed the throwable it is
660-
// being attached to, and that would escape this method and replace the very failure it is
661-
// supposed to preserve. Reachable since this also cleans up after a failed initialize():
662-
// initializeBlocking unwraps the ExecutionException and rethrows the factory's OWN exception
663-
// instance, so a factory that fails initialize() and close() with one latched exception hands
664-
// the same object back twice. The probe path could not do this — it always mints a fresh one.
659+
// Identity-guarded because addSuppressed rejects self-suppression — IllegalArgumentException
660+
// ("Self-suppression not permitted"), and it validates the argument even when suppression is
661+
// disabled — and that exception would escape this method and replace the very failure it is
662+
// supposed to preserve. Deliberately STRICTER than try-with-resources, whose JLS 14.20.3.1
663+
// translation calls addSuppressed unguarded (as does javac's lowering of it), so the language
664+
// construct propagates that IllegalArgumentException itself. This guard is therefore not
665+
// redundant with try-with-resources and must not be deleted as such.
666+
//
667+
// Reachable because a factory can hand the same throwable back twice: initializeBlocking unwraps
668+
// the ExecutionException and rethrows the factory's OWN exception instance, so one latched
669+
// failure out of both initialize() and close() is one object. The probe path can reach it too,
670+
// but only through TlsHandle.dispose(), whose unchecked throwables propagate unwrapped —
671+
// probe()'s own throws are all freshly minted.
665672
if (closeFailure != failure) {
666673
failure.addSuppressed(closeFailure);
667674
}

0 commit comments

Comments
 (0)