Message-ID: <gh-pgjdbc-pgjdbc-4160@github.com>
From: "vlsi (@vlsi)" <noreply+vlsi@github.com>
To: "pgjdbc/pgjdbc" <noreply+pgjdbc-pgjdbc@github.com>
Date: Mon, 08 Jun 2026 11:45:56 +0000
Subject: [pgjdbc/pgjdbc] PR #4160: refactor: favour composition over inheritance for Driver.ConnectTask
List-Id: <gh-pgjdbc-pgjdbc.github.com>
X-GitHub-Additions: 14
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: 16
X-GitHub-Head-Branch: refactor-connecttask-composition
X-GitHub-Head-SHA: 0375fe1fee1ca5c61b66cca56175ed9980ccb492
X-GitHub-Issue: 4160
X-GitHub-Merge-SHA: a0045fdd8644879a6c71a1aac606a2f0df9f81f0
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/4160
Content-Type: text/plain; charset=utf-8

### Why

Follow-up to the review on #4120 ([discussion](https://github.com/pgjdbc/pgjdbc/pull/4120#discussion_r3368161503)).

After #4120, `ConnectTask` extended `FutureTask` and needed a private chained constructor to thread the `abandoned` flag and the `establishedConnection` holder into the `super(...)` `Callable`. That indirection exists only because instance fields cannot be referenced before `super()` runs, so the state had to be created in one constructor and passed to another.

### What

Hold the `FutureTask` in a field instead of extending it, so the `Callable` captures the instance directly:

- one constructor replaces the private chained pair;
- `abandoned` becomes a plain `volatile boolean` (only ever set to `true`);
- `establishedConnection` is initialised at its declaration;
- `ConnectTask implements Runnable` and delegates `run()` to the field, so callers pass it straight to the `ThreadFactory`.

Behaviour is unchanged: `cancel(true)` and `get(...)` still run on the same `FutureTask`, and `abandoned` plus `establishedConnection` keep cancellation leak-free.

### How to verify

Against a local PostgreSQL 16:

- `./gradlew :postgresql:test --tests "org.postgresql.test.jdbc2.LoginTimeoutInterruptTest"` — passes; confirms `cancel(true)` still interrupts the worker out of `Thread.sleep`.
- `./gradlew :postgresql:test --tests "org.postgresql.test.jdbc2.ConnectThreadFactoryTest"` — passes.
- `./gradlew :postgresql:checkstyleMain :postgresql:autostyleJavaCheck` — passes.

### Changes to Existing Features:

* [ ] Does this break existing behaviour? No — pure refactor; the connection-attempt and abandonment semantics are preserved.
* [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? Existing tests from #4120 cover this path; no behaviour change to test.
* [x] Have you successfully run tests with your changes locally?

🤖 Generated with [Claude Code](https://claude.com/claude-code)

diff --git a/pgjdbc/src/main/java/org/postgresql/Driver.java b/pgjdbc/src/main/java/org/postgresql/Driver.java
index 33c24e63eb..d0ccf88779 100644
--- a/pgjdbc/src/main/java/org/postgresql/Driver.java
+++ b/pgjdbc/src/main/java/org/postgresql/Driver.java
@@ -44,7 +44,6 @@
 import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -353,26 +352,25 @@ private void setupLoggerFromProperties(@SuppressWarnings("UnusedVariable") Prope
    * immediately, the worker closes any connection it manages to establish after abandonment so that
    * it does not leak.</p>
    */
-  private static class ConnectTask extends FutureTask<Connection> {
-    private final AtomicBoolean abandoned;
-    private final AtomicReference<@Nullable Connection> establishedConnection;
+  private static class ConnectTask implements Runnable {
+    private volatile boolean abandoned;
+    private final AtomicReference<@Nullable Connection> establishedConnection = new AtomicReference<>();
+    private final FutureTask<Connection> futureTask;
 
     ConnectTask(String url, Properties props) {
-      this(new AtomicBoolean(), new AtomicReference<>(), url, props);
-    }
-
-    private ConnectTask(AtomicBoolean abandoned, AtomicReference<@Nullable Connection> establishedConnection,
-        String url, Properties props) {
-      super(() -> {
+      this.futureTask = new FutureTask<>(() -> {
         Connection conn = makeConnection(url, props);
         establishedConnection.set(conn);
-        if (abandoned.get() && establishedConnection.compareAndSet(conn, null)) {
+        if (abandoned && establishedConnection.compareAndSet(conn, null)) {
           closeConnection(conn);
         }
         return conn;
       });
-      this.abandoned = abandoned;
-      this.establishedConnection = establishedConnection;
+    }
+
+    @Override
+    public void run() {
+      futureTask.run();
     }
 
     /**
@@ -385,7 +383,7 @@ private ConnectTask(AtomicBoolean abandoned, AtomicReference<@Nullable Connectio
      */
     Connection getResult(long timeout) throws SQLException {
       try {
-        return get(timeout, TimeUnit.MILLISECONDS);
+        return futureTask.get(timeout, TimeUnit.MILLISECONDS);
       } catch (TimeoutException te) {
         abandon();
         throw new PSQLException(GT.tr("Connection attempt timed out."),
@@ -413,8 +411,8 @@ Connection getResult(long timeout) throws SQLException {
     }
 
     private void abandon() {
-      abandoned.set(true);
-      cancel(true);
+      abandoned = true;
+      futureTask.cancel(true);
       closeConnection(establishedConnection.getAndSet(null));
     }