Message-ID: From: "vlsi (@vlsi)" To: "pgjdbc/pgjdbc" Date: Thu, 04 Jun 2026 19:35:43 +0000 Subject: [pgjdbc/pgjdbc] PR #4156: fix: avoid nulling contextClassLoader on shared commonPool workers List-Id: X-GitHub-Additions: 53 X-GitHub-Author-Id: 213894 X-GitHub-Author-Login: vlsi X-GitHub-Base: master X-GitHub-Changed-Files: 2 X-GitHub-Commits: 1 X-GitHub-Deletions: 36 X-GitHub-Head-Branch: claude/elegant-napier-0bff06 X-GitHub-Head-SHA: 6fff1573c6e93764f1e79e6c9c1f626ca7ea5adb X-GitHub-Issue: 4156 X-GitHub-Labels: bug X-GitHub-Merge-SHA: 753a99d41d018e18e9ac88175ee652bb3922d746 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/4156 Content-Type: text/plain; charset=utf-8 ## Why The cleanup task in `LazyCleanerImpl` (the Java 8 variant) submits a task to `ForkJoinPool.commonPool()` and set the worker's `contextClassLoader` to `null` without ever restoring it. Common-pool workers are shared across the whole JVM and reused, so an unrelated task scheduled later on the same worker ran with a `null` `contextClassLoader`. Libraries that rely on the thread context classloader then break — the reporter hit this with reflections8. See [Issue #4155](https://github.com/pgjdbc/pgjdbc/issues/4155). The `null` was added for [Issue #3953](https://github.com/pgjdbc/pgjdbc/issues/3953): a guard against a long-lived worker pinning the submitter's classloader (for example a web application's). ## What - `LazyCleanerImpl` (Java 8 variant) now clears the `contextClassLoader` on the **submitting** thread only, around the `execute()` call, and restores it immediately afterwards. If the common pool spawns a fresh worker for the cleanup task, the worker is not born holding the caller's classloader, so the #3953 leak is still prevented — but the shared worker's `contextClassLoader` is never mutated, so #4155 is fixed. - The in-task `setContextClassLoader` on the worker is gone, which also removes the `InnocuousForkJoinWorkerThread` `SecurityException` path from #3953. - No change to the Java 11+ variant: it uses the native `Cleaner` and never touches the context classloader. - Changelog entry under 42.7.12. ## How to verify The cleanup `ForkJoinPool` path runs only under the Java 8 variant, so verification uses `-PjdkTestVersion=8`: - `./gradlew -PjdkTestVersion=8 :pgjdbc-junit4-test:test --tests org.postgresql.test.jdbc4.jdbc41.DriverSupportsClassUnloadingTest` — green. This classloader-unloading test guards the #3953 leak; it fails if the worker is left holding the submitter's classloader. - `./gradlew -PjdkTestVersion=8 :postgresql:test --tests org.postgresql.util.LazyCleanerTest` — green. - `./gradlew -PenableCheckerframework :postgresql:classes :postgresql:checkstyleMain` — passes (nullness + style). ## Note An earlier revision instead restored the **worker's** `contextClassLoader` in a `finally` block. That reintroduced the #3953 classloader leak and failed `DriverSupportsClassUnloadingTest` on Java 8, because restoring the value re-pinned the submitter's classloader on the long-lived worker. Moving the scrub to the submitting thread avoids both problems. The #4155 direction (a `null` loader leaking to unrelated tasks) has no direct regression test, since it would need a custom common-pool thread factory installed at JVM start. 🤖 Generated with [Claude Code](https://claude.com/claude-code) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4137b06225..2594a79d8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). * chore: `commitPrepared` / `rollback`-of-prepared now return `XAER_RMFAIL` instead of `XAER_RMERR` when the underlying connection is left in a non-idle `TransactionState`. Transaction managers (Geronimo, Narayana, Atomikos) treat `XAER_RMFAIL` as retryable on a fresh `XAResource`; the prepared transaction is no longer abandoned. ### Fixed +* fix: the driver no longer nulls the `contextClassLoader` of shared `ForkJoinPool.commonPool()` worker threads, which previously left unrelated tasks on those threads running with a `null` classloader [Issue #4155](https://github.com/pgjdbc/pgjdbc/issues/4155) * fix: getCharacterStream wraps String in StringReader [PR #4063](https://github.com/pgjdbc/pgjdbc/pull/4063) * fix: `PGXAConnection` no longer saves and restores the underlying connection's JDBC `autoCommit` flag. All XA-protocol SQL (`BEGIN`, `PREPARE TRANSACTION`, `COMMIT`, `ROLLBACK`, `COMMIT PREPARED`, `ROLLBACK PREPARED`, the `recover()` SELECT) is sent through `QUERY_SUPPRESS_BEGIN`, so the caller's `autoCommit` value is invariant across every `XAResource` call. Fixes the "2nd phase commit must be issued using an idle connection" failure during recovery on managed datasources that pool connections with `autoCommit=false` (TomEE, WildFly, WebSphere Liberty). * fix: `PGXAConnection.prepare()` now mutates XA state only after `PREPARE TRANSACTION` succeeds. A failed `PREPARE` previously left the driver thinking the branch was already prepared, so the follow-up `rollback(xid)` tried `ROLLBACK PREPARED` against a non-existent gid and returned `XAER_RMERR`. Transaction managers (Narayana) escalated this to `HeuristicMixedException`. With the fix, `rollback(xid)` takes the active-branch path and issues a plain `ROLLBACK`, which the server accepts cleanly. Fixes [Issue #3153](https://github.com/pgjdbc/pgjdbc/issues/3153), [Issue #3123](https://github.com/pgjdbc/pgjdbc/issues/3123). diff --git a/pgjdbc/src/main/java/org/postgresql/util/LazyCleanerImpl.java b/pgjdbc/src/main/java/org/postgresql/util/LazyCleanerImpl.java index 6e6fbfa677..1b20c7ddf1 100644 --- a/pgjdbc/src/main/java/org/postgresql/util/LazyCleanerImpl.java +++ b/pgjdbc/src/main/java/org/postgresql/util/LazyCleanerImpl.java @@ -176,45 +176,61 @@ private boolean startThread() { // We use ForkJoinPool to work around Thread.inheritedAccessControlContext memory leak // Java creates FJP threads without caller's access control context, thus we reduce // the surface for the leak. - ForkJoinPool.commonPool().execute( - () -> { - // ForkJoinPool does not inherit contextClassLoader from the submitting thread, - // however custom thread factories might, so we try nullifying it to avoid - // classloader leaks. InnocuousForkJoinWorkerThread forbids setContextClassLoader, - // so we catch SecurityException to avoid crashing the cleanup thread. - // See https://github.com/pgjdbc/pgjdbc/issues/3953 - try { - Thread.currentThread().setContextClassLoader(null); - } catch (SecurityException ignore) { - // InnocuousForkJoinWorkerThread or a SecurityManager forbids setContextClassLoader - } - RefQueueBlocker blocker = - new RefQueueBlocker<>(queue, threadName, threadTtl); - while (true) { - try { - ForkJoinPool.managedBlock(blocker); - Node ref = (Node) blocker.drainOne(); - if (ref != null) { - ref.onClean(true); - } else if (checkEmpty()) { - // The blocker waited the full threadTtl without receiving a ref, and there are - // no pending registrations, so the cleanup task can terminate. Keeping the task - // alive across transient empties amortizes ForkJoinPool submit/compensation - // overhead under bursty workloads. - break; - } - } catch (InterruptedException e) { - if (checkEmpty()) { - LOGGER.log(Level.FINE, "Got interrupt and the cleanup queue is empty, will terminate the cleanup thread"); - break; + // + // Null this (submitting) thread's contextClassLoader around execute(): if the common pool + // spawns a fresh worker to run the cleanup task, the worker inherits the submitter's + // contextClassLoader at birth. Leaving that classloader (which may be a web application's) + // on a worker that lives for the JVM's lifetime would pin it and leak it + // (https://github.com/pgjdbc/pgjdbc/issues/3953). We restore the value right after, so the + // change is scoped to our own call: we never mutate a shared common-pool worker, whose + // contextClassLoader unrelated tasks rely on (https://github.com/pgjdbc/pgjdbc/issues/4155). + Thread current = Thread.currentThread(); + ClassLoader prevContextCL = current.getContextClassLoader(); + try { + current.setContextClassLoader(null); + } catch (SecurityException ignore) { + // setContextClassLoader can be denied even without a SecurityManager: when this is an + // InnocuousForkJoinWorkerThread (a registration made from a common-pool task), it throws + // unconditionally. This is best-effort, so submit anyway with the original loader. + } + try { + ForkJoinPool.commonPool().execute( + () -> { + RefQueueBlocker blocker = + new RefQueueBlocker<>(queue, threadName, threadTtl); + while (true) { + try { + ForkJoinPool.managedBlock(blocker); + Node ref = (Node) blocker.drainOne(); + if (ref != null) { + ref.onClean(true); + } else if (checkEmpty()) { + // The blocker waited the full threadTtl without receiving a ref, and there are + // no pending registrations, so the cleanup task can terminate. Keeping the task + // alive across transient empties amortizes ForkJoinPool submit/compensation + // overhead under bursty workloads. + break; + } + } catch (InterruptedException e) { + if (checkEmpty()) { + LOGGER.log(Level.FINE, "Got interrupt and the cleanup queue is empty, will terminate the cleanup thread"); + break; + } + LOGGER.log(Level.FINE, "Got interrupt and the cleanup queue is NOT empty. Will ignore the interrupt"); + } catch (Throwable e) { + LOGGER.log(Level.WARNING, "Unexpected exception while executing onClean", e); } - LOGGER.log(Level.FINE, "Got interrupt and the cleanup queue is NOT empty. Will ignore the interrupt"); - } catch (Throwable e) { - LOGGER.log(Level.WARNING, "Unexpected exception while executing onClean", e); } } - } - ); + ); + } finally { + try { + current.setContextClassLoader(prevContextCL); + } catch (SecurityException ignore) { + // Denied by an InnocuousForkJoinWorkerThread or a SecurityManager, as above; the loader + // was never changed, so there is nothing to restore. + } + } return true; }