Message-ID: From: "vlsi (@vlsi)" To: "pgjdbc/pgjdbc" Date: Sun, 31 May 2026 16:51:29 +0000 Subject: [pgjdbc/pgjdbc] PR #4121: test: stabilise StatementTest.fastCloses on Windows List-Id: X-GitHub-Additions: 40 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: 51 X-GitHub-Head-Branch: claude/wizardly-noyce-30d399 X-GitHub-Head-SHA: 6ccd9f135ccbf2d5a9ef7f2e8a336bb055947701 X-GitHub-Issue: 4121 X-GitHub-Labels: building-and-testing X-GitHub-Merge-SHA: 472632250c068158bfd57aff192c831347ac0160 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/4121 Content-Type: text/plain; charset=utf-8 ### What Make `StatementTest.fastCloses()` bound its work by wall-clock time instead of a fixed 1000 iterations, and clean up dead setup the test never used. Also apply an overflow-safe `nanoTime()` comparison to the neighbouring `sideStatementFinalizers()`. ### Why `fastCloses()` intermittently hit its `@Timeout(40)` on Windows CI (for example [run 26582831581](https://github.com/pgjdbc/pgjdbc/actions/runs/26582831581/job/78320413863)). The test creates a fresh `Statement` per iteration and races `st.close()` (on another thread) against `st.executeQuery("select 1")`. When a close lands while the query is still in flight it cancels the query, and a cancel opens a **fresh socket** to the backend — the wire protocol mandates a separate connection for cancel requests (`QueryExecutorBase.sendQueryCancel`). Many short-lived sockets are expensive on Windows (slower connection setup, `TIME_WAIT` accumulation), so a fixed 1000-iteration loop can run past the 40s timeout. The main DB connection is already reused; the churn comes from the cancel path, which cannot reuse a connection. ### How - Bound the loop by a 10s wall-clock budget (cap still 1000) so the close/execute race runs on every iteration but stops well inside the timeout on slow runners. - Use the overflow-safe `System.nanoTime() - start < budget` form rather than `nanoTime() < deadline`, which can misbehave across a `nanoTime()` wrap-around. - Shut down and await the executor in a `finally` block so background close tasks never leak into other tests. - Drop copy-paste setup `fastCloses` never exercised: the `notify_then_sleep` function, the `client_min_messages` session setting, and the unused `cnt` histogram. These came from the adjacent `closeInProgressStatement` test, whose query actually calls the function; `fastCloses` has always run `select 1` (dead since the original 2017 commit that fixed #1022). `notify_then_sleep` remains in use by `warningsAreAvailableAsap` and `closeInProgressStatement`. - Apply the same overflow-safe `nanoTime` comparison to `sideStatementFinalizers`. ### How to verify ``` ./gradlew :postgresql:test --tests 'org.postgresql.test.jdbc2.StatementTest' ``` Local run (PostgreSQL 16): full `StatementTest` 43/43 green; `fastCloses` ~1s, `sideStatementFinalizers` ~2s (its budget). `compileTestJava` and `checkstyleTest` pass. ### All Submissions: * [x] Have you followed the guidelines in our [Contributing](https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md) document? * [x] Have you checked to ensure there aren't other open [Pull Requests](../../pulls) for the same update/change? ### Changes to Existing Features: * [x] Does this break existing behaviour? No — test-only change, no production code touched. * [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? N/A — this stabilises existing tests. * [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/test/jdbc2/StatementTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java index ba10ef78b2..3e9ba6e484 100644 --- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java +++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java @@ -41,9 +41,7 @@ import java.sql.Statement; import java.time.Duration; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Properties; import java.util.Random; import java.util.concurrent.Callable; @@ -1007,59 +1005,48 @@ void concurrentIsValid() throws Throwable { @Test @Timeout(40) - void fastCloses() throws SQLException { + void fastCloses() throws Exception { + // Closing a Statement from another thread while executeQuery() runs must never deadlock or + // hang. A close that lands while the query is still in flight cancels it, and every cancel + // opens a fresh socket to the backend (the protocol mandates a separate connection for cancel + // requests). Short-lived sockets are expensive on Windows, so the loop is bounded by wall-clock + // time rather than a fixed iteration count: the race runs on every iteration and stops once the + // time budget is spent, which keeps the test well inside the @Timeout on slow runners. ExecutorService executor = Executors.newSingleThreadExecutor(); - con.createStatement().execute("SET SESSION client_min_messages = 'NOTICE'"); - con.createStatement() - .execute("CREATE OR REPLACE FUNCTION notify_then_sleep() RETURNS VOID AS " - + "$BODY$ " - + "BEGIN " - + "RAISE NOTICE 'start';" - + "EXECUTE pg_sleep(1);" // Note: timeout value does not matter here, we just test if test crashes or locks somehow - + "END " - + "$BODY$ " - + "LANGUAGE plpgsql;"); - Map cnt = new HashMap<>(); - final Random rnd = new Random(); - for (int i = 0; i < 1000; i++) { - final Statement st = con.createStatement(); - executor.submit((Callable) () -> { - int s = rnd.nextInt(10); - if (s > 8) { - try { - Thread.sleep(0); - } catch (InterruptedException ex) { - // don't execute the close here as this thread was cancelled below in shutdownNow + try { + final Random rnd = new Random(); + final long start = System.nanoTime(); + final long budgetNanos = TimeUnit.SECONDS.toNanos(10); + // Subtract and compare to stay correct across a nanoTime() overflow; see its Javadoc. + int iterations = 0; + while (iterations < 1000 && System.nanoTime() - start < budgetNanos) { + iterations++; + try (Statement st = con.createStatement()) { + executor.submit((Callable) () -> { + if (rnd.nextInt(10) > 8) { + Thread.yield(); + } + st.close(); return null; + }); + st.executeQuery("select 1").close(); + // Acceptable: the close landed before the query started or after it finished. + } catch (SQLException e) { + // The close may cancel the in-flight query (QUERY_CANCELED) or close the statement + // before execution begins (OBJECT_NOT_IN_STATE); any other state is a failure. + String sqlState = e.getSQLState(); + if (!PSQLState.OBJECT_NOT_IN_STATE.getState().equals(sqlState) + && !PSQLState.QUERY_CANCELED.getState().equals(sqlState)) { + fail("Query is expected to succeed or be cancelled via st.close(), got SQLState " + + sqlState + ": " + e.getMessage()); } } - st.close(); - return null; - }); - ResultSet rs = null; - String sqlState = "0"; - try { - rs = st.executeQuery("select 1"); - // Acceptable - } catch (SQLException e) { - sqlState = e.getSQLState(); - if (!PSQLState.OBJECT_NOT_IN_STATE.getState().equals(sqlState) - && !PSQLState.QUERY_CANCELED.getState().equals(sqlState)) { - assertEquals( - PSQLState.QUERY_CANCELED.getState(), - e.getSQLState(), - "Query is expected to be cancelled via st.close(), got " + e.getMessage() - ); - } - } finally { - TestUtil.closeQuietly(rs); - TestUtil.closeQuietly(st); } - Integer val = cnt.get(sqlState); - val = (val == null ? 0 : val) + 1; - cnt.put(sqlState, val); + assertTrue(iterations > 0, "fastCloses ran no iterations"); + } finally { + executor.shutdown(); + executor.awaitTermination(10, TimeUnit.SECONDS); } - executor.shutdown(); } /** @@ -1068,7 +1055,8 @@ void fastCloses() throws SQLException { */ @Test void sideStatementFinalizers() throws SQLException { - long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(2); + final long start = System.nanoTime(); + final long budgetNanos = TimeUnit.SECONDS.toNanos(2); final AtomicInteger leaks = new AtomicInteger(); final AtomicReference cleanupFailure = new AtomicReference<>(); @@ -1078,7 +1066,8 @@ void sideStatementFinalizers() throws SQLException { cleaners.add(new LazyCleanerImpl("pgjdbc-test-cleaner-" + i, Duration.ofSeconds(2))); } - for (int q = 0; System.nanoTime() < deadline || leaks.get() < 10000; q++) { + // Subtract and compare to stay correct across a nanoTime() overflow; see its Javadoc. + for (int q = 0; System.nanoTime() - start < budgetNanos || leaks.get() < 10000; q++) { for (int i = 0; i < 100; i++) { PreparedStatement ps = con.prepareStatement("select " + (i + q)); ps.close();