Message-ID: From: "vlsi (@vlsi)" To: "pgjdbc/pgjdbc" Date: Sun, 31 May 2026 18:22:40 +0000 Subject: [pgjdbc/pgjdbc] PR #4122: test: fix LazyCleanerTest timeouts for the lingering Java 8 cleanup thread List-Id: X-GitHub-Additions: 59 X-GitHub-Author-Id: 213894 X-GitHub-Author-Login: vlsi X-GitHub-Base: master X-GitHub-Changed-Files: 1 X-GitHub-Commits: 1 X-GitHub-Deletions: 18 X-GitHub-Head-Branch: claude/objective-sinoussi-e89694 X-GitHub-Head-SHA: 57280d8537815ba653fb428efd3e898d6da34797 X-GitHub-Issue: 4122 X-GitHub-Labels: building-and-testing X-GitHub-Merge-SHA: 301cfb7a5a1950eb9d545dcb4b8d13162bd4e855 X-GitHub-Merged-By: vlsi X-GitHub-Repo: pgjdbc/pgjdbc X-GitHub-State: merged X-GitHub-Type: pull_request X-GitHub-Url: https://github.com/pgjdbc/pgjdbc/pull/4122 Content-Type: text/plain; charset=utf-8 ### What Adjust the `LazyCleanerTest` await budgets and make its thread-lifecycle assertions adapt to the actually loaded cleaner implementation. No production code changes. - Derive the thread-stop budget from `threadTtl` (`2 × ttl + 5s`) via a small `threadStopBudget` helper, instead of a hard-coded `10s`. - Raise the cleanup-completion budget in `exceptionsDuringCleanupAreHandled` from `10s` to `30s`. - Branch the thread-lifecycle assertions on the observed behaviour (`isThreadRunning()` captured after `register()`) rather than on the runtime Java version: the dedicated-thread variant must stop once the queue empties; the native-`Cleaner` variant never starts a thread. - Apply the same `threadStopBudget` formula to `phantomCleaner` to remove the same latent risk. ### Why `LazyCleanerImpl` now keeps its Java 8 cleanup thread alive for up to one `threadTtl` after the reference queue empties (05166d0fd, #4037). The worst case to observe `!isThreadRunning()` is therefore about `2 × threadTtl`: up to one window to pick up the final GC'd reference, plus a full window that must time out before the loop breaks. The tests used `threadTtl = 5s` with an `Await` budget of exactly `10s = 2 × ttl` and zero slack. Once the CI matrix started occasionally running under `-XX:ActiveProcessorCount=1` (e06c64995), GC, the reference handler, and the ForkJoinPool worker share a single core, and the extra latency pushed two tests past the deadline: - `cleanupCompletesAfterManualClean` — timed out waiting for `!isThreadRunning()`. - `exceptionsDuringCleanupAreHandled` — timed out waiting for both cleanups to run (this one is bound by GC/enqueue latency, not `threadTtl`, hence the separate `30s` budget). Reported failures: [run 26633780304](https://github.com/pgjdbc/pgjdbc/actions/runs/26633780304/job/78488849120) and [run 26715502613](https://github.com/pgjdbc/pgjdbc/actions/runs/26715502613/job/78733270691). The active `LazyCleanerImpl` is selected by multi-release packaging, not by the runtime JDK. The Maven source-distribution build runs the PhantomReference-based Java 8 variant on any JDK, so the assertions cannot branch on `JavaVersion` — they detect the variant from its behaviour instead. The production thread TTL (30s default) is unchanged; the tests keep their `5s` value on purpose, since a lifecycle test using the production TTL would need a >60s budget. ### How to verify Gradle, under a single CPU, on each JDK: ``` ./gradlew :postgresql:test --tests org.postgresql.util.LazyCleanerTest \ -PjdkTestVersion=8 -PtestExtraJvmArgs=-XX:ActiveProcessorCount=1 ./gradlew :postgresql:test --tests org.postgresql.util.LazyCleanerTest \ -PjdkTestVersion=11 -PtestExtraJvmArgs=-XX:ActiveProcessorCount=1 ``` Source distribution (the Java 8 variant on a modern JDK, the path that failed in CI): ``` ./gradlew sourceDistribution -Ppgjdbc.version=1.0 -Prelease -Psigning.pgp.enabled=OFF cd pgjdbc/build/distributions && tar xzf postgresql-1.0-jdbc-src.tar.gz cd postgresql-1.0-jdbc-src && mvn --batch-mode test -Dtest=LazyCleanerTest ``` Local results, all 4 tests pass: - Gradle Java 8 and source-distribution Maven (dedicated thread): ~15s (one `threadTtl` window per test, within the 15s budget). - Gradle Java 11 and Java 21 (native `Cleaner`): ~1.1s. ### Changes to Existing Features: * [ ] Does this break existing behaviour? No — test-only change; production `LazyCleanerImpl` is untouched. * [x] Have you added an explanation of what your changes do and why you'd like us to include them? * [x] Have you written new tests for your core changes, as applicable? Adjusted the existing tests; assertions now adapt to the loaded cleaner variant. * [x] Have you successfully run tests with your changes locally? 🤖 Generated with [Claude Code](https://claude.com/claude-code) diff --git a/pgjdbc/src/test/java/org/postgresql/util/LazyCleanerTest.java b/pgjdbc/src/test/java/org/postgresql/util/LazyCleanerTest.java index 9ba1850633..c1f1a7a778 100644 --- a/pgjdbc/src/test/java/org/postgresql/util/LazyCleanerTest.java +++ b/pgjdbc/src/test/java/org/postgresql/util/LazyCleanerTest.java @@ -24,6 +24,7 @@ import static java.time.Duration.ofSeconds; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -33,6 +34,7 @@ import org.junit.jupiter.api.Test; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -46,7 +48,8 @@ void phantomCleaner() throws InterruptedException { List list = new ArrayList<>(Arrays.asList( new Object(), new Object(), new Object())); - LazyCleanerImpl t = new LazyCleanerImpl("Cleaner", ofSeconds(5)); + Duration ttl = ofSeconds(5); + LazyCleanerImpl t = new LazyCleanerImpl("Cleaner", ttl); String[] collected = new String[list.size()]; List> cleaners = new ArrayList<>(); @@ -67,9 +70,13 @@ void phantomCleaner() throws InterruptedException { ); } + // The active LazyCleanerImpl is selected by multi-release packaging, not by the runtime JDK + // (the Maven source-distribution build runs the Java 8 variant on any JDK), so branch on the + // observed behaviour rather than on JavaVersion. + boolean dedicatedThread = t.isThreadRunning(); if (JavaVersion.getRuntimeVersion() == JavaVersion.v1_8) { - assertTrue(t.isThreadRunning(), - "cleanup thread should be running, and it should wait for the leaks"); + assertTrue(dedicatedThread, + "Java 8 always loads the PhantomReference-based cleaner, which runs a dedicated thread"); } cleaners.get(1).clean(); @@ -82,14 +89,14 @@ void phantomCleaner() throws InterruptedException { System.gc(); System.gc(); - if (JavaVersion.getRuntimeVersion() == JavaVersion.v1_8) { + if (dedicatedThread) { Await.until( - "The cleanup thread should detect leaks and terminate within 5-10 seconds after GC", - ofSeconds(10), + "The cleanup thread should detect leaks and terminate after GC", + threadStopBudget(ttl), () -> !t.isThreadRunning() ); } else { - // Allow some room for Java's Cleaner to clean the refs + // The native Cleaner has no dedicated thread to stop; give it room to process the refs Thread.sleep(1000); } @@ -103,7 +110,8 @@ void phantomCleaner() throws InterruptedException { @Test void cleanupCompletesAfterManualClean() throws InterruptedException { String threadName = UUID.randomUUID().toString(); - LazyCleanerImpl t = new LazyCleanerImpl(threadName, ofSeconds(5)); + Duration ttl = ofSeconds(5); + LazyCleanerImpl t = new LazyCleanerImpl(threadName, ttl); AtomicBoolean cleaned = new AtomicBoolean(); List list = new ArrayList<>(); @@ -115,9 +123,13 @@ void cleanupCompletesAfterManualClean() throws InterruptedException { leak -> cleaned.set(true) ); + // The active LazyCleanerImpl is selected by multi-release packaging, not by the runtime JDK + // (the Maven source-distribution build runs the Java 8 variant on any JDK), so branch on the + // observed behaviour rather than on JavaVersion. + boolean dedicatedThread = t.isThreadRunning(); if (JavaVersion.getRuntimeVersion() == JavaVersion.v1_8) { - assertTrue(t.isThreadRunning(), - "cleanup thread should be running when there are objects to monitor"); + assertTrue(dedicatedThread, + "Java 8 always loads the PhantomReference-based cleaner, which runs a dedicated thread"); } // Manually clean the object @@ -131,19 +143,23 @@ void cleanupCompletesAfterManualClean() throws InterruptedException { System.gc(); System.gc(); - if (JavaVersion.getRuntimeVersion() == JavaVersion.v1_8) { + if (dedicatedThread) { Await.until( "Cleanup thread should stop when no objects remain", - ofSeconds(10), + threadStopBudget(ttl), () -> !t.isThreadRunning() ); + } else { + assertFalse(t.isThreadRunning(), + "Native Cleaner variant: there is no dedicated thread to stop"); } } @Test @DisableLogger(LazyCleanerImpl.class) void exceptionsDuringCleanupAreHandled() throws InterruptedException { - LazyCleanerImpl t = new LazyCleanerImpl("test-cleaner", ofSeconds(5)); + Duration ttl = ofSeconds(5); + LazyCleanerImpl t = new LazyCleanerImpl("test-cleaner", ttl); java.util.concurrent.atomic.AtomicInteger cleanupCount = new java.util.concurrent.atomic.AtomicInteger(0); List list = new ArrayList<>(); @@ -166,9 +182,13 @@ void exceptionsDuringCleanupAreHandled() throws InterruptedException { leak -> secondCleaned.set(true) ); + // The active LazyCleanerImpl is selected by multi-release packaging, not by the runtime JDK + // (the Maven source-distribution build runs the Java 8 variant on any JDK), so branch on the + // observed behaviour rather than on JavaVersion. + boolean dedicatedThread = t.isThreadRunning(); if (JavaVersion.getRuntimeVersion() == JavaVersion.v1_8) { - assertTrue(t.isThreadRunning(), - "cleanup thread should be running when there are objects to monitor"); + assertTrue(dedicatedThread, + "Java 8 always loads the PhantomReference-based cleaner, which runs a dedicated thread"); } // Trigger cleanup @@ -176,18 +196,25 @@ void exceptionsDuringCleanupAreHandled() throws InterruptedException { System.gc(); System.gc(); + // Cleanup completion is bound by GC and reference-enqueue latency, not by threadTtl. Allow a + // generous budget so the assertion holds even when the JVM runs on a single CPU + // (-XX:ActiveProcessorCount=1), where GC, the reference handler, and the ForkJoinPool worker + // all share one core. Await.until( "Both cleanups should complete despite exception", - ofSeconds(10), + ofSeconds(30), () -> cleanupCount.get() == 1 && secondCleaned.get() ); - if (JavaVersion.getRuntimeVersion() == JavaVersion.v1_8) { + if (dedicatedThread) { Await.until( "Cleanup thread should stop after all objects are cleaned", - ofSeconds(10), + threadStopBudget(ttl), () -> !t.isThreadRunning() ); + } else { + assertFalse(t.isThreadRunning(), + "Native Cleaner variant: there is no dedicated thread to stop"); } } @@ -204,4 +231,18 @@ void exceptionOnCleanRethrowsToCaller() { RuntimeException thrownException = assertThrows(RuntimeException.class, cleanable::clean); assertSame(expectedException, thrownException, "Expected same exception instance to be thrown"); } + + /** + * Worst-case time for the Java 8 cleanup thread to stop once its registry empties. The thread + * waits up to one {@code threadTtl} window to observe the final GC'd reference, then must time + * out a second full {@code threadTtl} window before the loop breaks (see {@code LazyCleanerImpl}). + * The extra seconds absorb scheduling slack when the JVM runs on a single CPU + * (-XX:ActiveProcessorCount=1). + * + * @param threadTtl the time-to-live configured for the cleaner + * @return the await budget to use for {@code !isThreadRunning()} + */ + private static Duration threadStopBudget(Duration threadTtl) { + return threadTtl.multipliedBy(2).plus(ofSeconds(5)); + } }