pgjdbc/pgjdbc GitHub issues and pull requests (mirror)  

help / color / mirror / Atom feed

From: vlsi (@vlsi) <[email protected]>
To: pgjdbc/pgjdbc <[email protected]>
Subject: [pgjdbc/pgjdbc] PR #4160: refactor: favour composition over inheritance for Driver.ConnectTask
Date: Mon, 08 Jun 2026 11:45:56 +0000
Message-ID: <[email protected]> (raw)

### 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));
     }
 

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: github://pgjdbc/pgjdbc
  Cc: [email protected], [email protected]
  Subject: Re: [pgjdbc/pgjdbc] PR #4160: refactor: favour composition over inheritance for Driver.ConnectTask
  In-Reply-To: <<[email protected]>>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox