pgjdbc/pgjdbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feedFrom: vlsi (@vlsi) <[email protected]>
To: pgjdbc/pgjdbc <[email protected]>
Subject: [pgjdbc/pgjdbc] PR #4161: fix: close socket when PgConnection setup fails after connect
Date: Mon, 08 Jun 2026 13:14:24 +0000
Message-ID: <[email protected]> (raw)
### Why
`PgConnection`'s constructor opens the network connection with `ConnectionFactory.openConnection(...)`, then runs a long sequence of setup steps — `SET SESSION CHARACTERISTICS`, `COMMIT`/`ROLLBACK` query preparation, `initObjectTypes`, type-cache setup, and so on — before registering the connection with the `Cleaner` on the constructor's last line.
If any of those steps fails (a server error on a `SET`, a mid-setup network drop surfacing as `IOException`/`SQLException`, a `RuntimeException`, or a thread interrupt at an interruptible point), the constructor throws without ever closing `queryExecutor`. The half-built `PgConnection` is never assigned to a variable (`Driver.makeConnection` just does `return new PgConnection(...)`) and the `Cleaner` has not been registered yet, so nothing closes the open socket and it leaks.
The cleanup inside `ConnectionFactoryImpl` (`tryConnect` / `openConnectionImpl`, which call `closeStream(...)` on failure) only covers failures *inside* the factory. The gap is the `PgConnection`-level setup that runs after the factory returns. This is a pre-existing issue, independent of the recent loginTimeout / `connectThreadFactory` work.
### What
- Wrap the constructor body that runs after `ConnectionFactory.openConnection(...)` in a `try`/`catch (SQLException | RuntimeException | Error e)` that closes `queryExecutor` and rethrows the original failure, attaching any close error as a suppressed exception (mirroring the `closeStream(stream, e)` pattern in `ConnectionFactoryImpl`).
- `QueryExecutorBase.close()` checks `isClosed()`, so closing again on a path that already closed the executor is a safe no-op — there is no double-close hazard.
- Add `ConnectionSetupFailureTest`. It forces a failure *after* connect with an invalid `stringtype` value (validated only inside the constructor, after the socket is open) and asserts, through a small nested recording `SocketFactory`, that every socket handed out is closed.
### How to verify
```
./gradlew --quiet :postgresql:compileJava
./gradlew --quiet :postgresql:checkstyleMain :postgresql:autostyleJavaCheck
./gradlew --quiet :postgresql:test --tests "org.postgresql.test.jdbc2.ConnectionSetupFailureTest"
```
The new test fails on `master` (the socket to the server stays open) and passes with this change. The existing `ConnectionTest` suite continues to pass.
### Changes to Existing Features
* This does not break existing behaviour: on the happy path the `try` completes normally and the constructor is unchanged. Only the failure path is affected — it now closes the socket instead of leaking it.
### New Feature Submissions
1. [x] Does your submission pass tests?
2. [x] Does `./gradlew styleCheck` pass?
3. [ ] N/A — tests are discovered by JUnit 5; this module no longer uses hand-maintained test suites.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
index 1e03d1b4bd..c6351adacc 100644
--- a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
+++ b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
@@ -289,123 +289,137 @@ public PgConnection(HostSpec[] hostSpecs,
// Now make the initial connection and set up local state
this.queryExecutor = ConnectionFactory.openConnection(hostSpecs, info);
- // WARNING for unsupported servers (9.0 and lower are not supported)
- if (LOGGER.isLoggable(Level.WARNING) && !haveMinimumServerVersion(ServerVersion.v9_1)) {
- LOGGER.log(Level.WARNING, "Unsupported Server Version: {0}", queryExecutor.getServerVersion());
- }
+ // The socket is open now, but the connection is only registered with the Cleaner at the very
+ // end of this constructor. If any setup step below fails, close queryExecutor explicitly so
+ // the socket is not leaked, then rethrow the original failure.
+ try {
+ // WARNING for unsupported servers (9.0 and lower are not supported)
+ if (LOGGER.isLoggable(Level.WARNING) && !haveMinimumServerVersion(ServerVersion.v9_1)) {
+ LOGGER.log(Level.WARNING, "Unsupported Server Version: {0}", queryExecutor.getServerVersion());
+ }
- setSessionReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY", false, true);
- setSessionNotReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ WRITE", false, true);
+ setSessionReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY", false, true);
+ setSessionNotReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ WRITE", false, true);
- // Set read-only early if requested
- if (PGProperty.READ_ONLY.getBoolean(info)) {
- setReadOnly(true);
- }
+ // Set read-only early if requested
+ if (PGProperty.READ_ONLY.getBoolean(info)) {
+ setReadOnly(true);
+ }
- this.hideUnprivilegedObjects = PGProperty.HIDE_UNPRIVILEGED_OBJECTS.getBoolean(info);
+ this.hideUnprivilegedObjects = PGProperty.HIDE_UNPRIVILEGED_OBJECTS.getBoolean(info);
- // Default is true: DDL transparently invalidates the prepared-statement
- // cache so the driver re-prepares server plans rather than surfacing
- // "cached plan must not change result type" to callers.
- queryExecutor.setFlushCacheOnDdl(PGProperty.FLUSH_CACHE_ON_DDL.getBoolean(info));
+ // Default is true: DDL transparently invalidates the prepared-statement
+ // cache so the driver re-prepares server plans rather than surfacing
+ // "cached plan must not change result type" to callers.
+ queryExecutor.setFlushCacheOnDdl(PGProperty.FLUSH_CACHE_ON_DDL.getBoolean(info));
- // get oids that support binary transfer
- Set<Integer> binaryOids = getBinaryEnabledOids(info);
- // get oids that should be disabled from transfer
- binaryDisabledOids = getBinaryDisabledOids(info);
- // if there are any, remove them from the enabled ones
- if (!binaryDisabledOids.isEmpty()) {
- binaryOids.removeAll(binaryDisabledOids);
- }
+ // get oids that support binary transfer
+ Set<Integer> binaryOids = getBinaryEnabledOids(info);
+ // get oids that should be disabled from transfer
+ binaryDisabledOids = getBinaryDisabledOids(info);
+ // if there are any, remove them from the enabled ones
+ if (!binaryDisabledOids.isEmpty()) {
+ binaryOids.removeAll(binaryDisabledOids);
+ }
- // split for receive and send for better control
- Set<Integer> useBinarySendForOids = new HashSet<>(binaryOids);
+ // split for receive and send for better control
+ Set<Integer> useBinarySendForOids = new HashSet<>(binaryOids);
- Set<Integer> useBinaryReceiveForOids = new HashSet<>(binaryOids);
+ Set<Integer> useBinaryReceiveForOids = new HashSet<>(binaryOids);
- /*
- * Does not pass unit tests because unit tests expect setDate to have millisecond accuracy
- * whereas the binary transfer only supports date accuracy.
- */
- useBinarySendForOids.remove(Oid.DATE);
+ /*
+ * Does not pass unit tests because unit tests expect setDate to have millisecond accuracy
+ * whereas the binary transfer only supports date accuracy.
+ */
+ useBinarySendForOids.remove(Oid.DATE);
- queryExecutor.setBinaryReceiveOids(useBinaryReceiveForOids);
- queryExecutor.setBinarySendOids(useBinarySendForOids);
+ queryExecutor.setBinaryReceiveOids(useBinaryReceiveForOids);
+ queryExecutor.setBinarySendOids(useBinarySendForOids);
- if (LOGGER.isLoggable(Level.FINEST)) {
- LOGGER.log(Level.FINEST, " types using binary send = {0}", oidsToString(useBinarySendForOids));
- LOGGER.log(Level.FINEST, " types using binary receive = {0}", oidsToString(useBinaryReceiveForOids));
- LOGGER.log(Level.FINEST, " integer date/time = {0}", queryExecutor.getIntegerDateTimes());
- }
+ if (LOGGER.isLoggable(Level.FINEST)) {
+ LOGGER.log(Level.FINEST, " types using binary send = {0}", oidsToString(useBinarySendForOids));
+ LOGGER.log(Level.FINEST, " types using binary receive = {0}", oidsToString(useBinaryReceiveForOids));
+ LOGGER.log(Level.FINEST, " integer date/time = {0}", queryExecutor.getIntegerDateTimes());
+ }
- //
- // String -> text or unknown?
- //
+ //
+ // String -> text or unknown?
+ //
- String stringType = PGProperty.STRING_TYPE.getOrDefault(info);
- if (stringType != null) {
- if ("unspecified".equalsIgnoreCase(stringType)) {
- bindStringAsVarchar = false;
- } else if ("varchar".equalsIgnoreCase(stringType)) {
- bindStringAsVarchar = true;
+ String stringType = PGProperty.STRING_TYPE.getOrDefault(info);
+ if (stringType != null) {
+ if ("unspecified".equalsIgnoreCase(stringType)) {
+ bindStringAsVarchar = false;
+ } else if ("varchar".equalsIgnoreCase(stringType)) {
+ bindStringAsVarchar = true;
+ } else {
+ throw new PSQLException(
+ GT.tr("Unsupported value for stringtype parameter: {0}", stringType),
+ PSQLState.INVALID_PARAMETER_VALUE);
+ }
} else {
- throw new PSQLException(
- GT.tr("Unsupported value for stringtype parameter: {0}", stringType),
- PSQLState.INVALID_PARAMETER_VALUE);
+ bindStringAsVarchar = true;
}
- } else {
- bindStringAsVarchar = true;
- }
-
- // Initialize timestamp stuff
- timestampUtils = new TimestampUtils(!queryExecutor.getIntegerDateTimes(),
- new QueryExecutorTimeZoneProvider(queryExecutor));
- // Initialize common queries.
- // isParameterized==true so full parse is performed and the engine knows the query
- // is not a compound query with ; inside, so it could use parse/bind/exec messages
- commitQuery = createQuery("COMMIT", false, true).query;
- rollbackQuery = createQuery("ROLLBACK", false, true).query;
+ // Initialize timestamp stuff
+ timestampUtils = new TimestampUtils(!queryExecutor.getIntegerDateTimes(),
+ new QueryExecutorTimeZoneProvider(queryExecutor));
- int unknownLength = PGProperty.UNKNOWN_LENGTH.getInt(info);
+ // Initialize common queries.
+ // isParameterized==true so full parse is performed and the engine knows the query
+ // is not a compound query with ; inside, so it could use parse/bind/exec messages
+ commitQuery = createQuery("COMMIT", false, true).query;
+ rollbackQuery = createQuery("ROLLBACK", false, true).query;
- // Initialize object handling
- @SuppressWarnings("argument")
- TypeInfo typeCache = createTypeInfo(this, unknownLength);
- this.typeCache = typeCache;
- initObjectTypes(info);
+ int unknownLength = PGProperty.UNKNOWN_LENGTH.getInt(info);
- if (PGProperty.LOG_UNCLOSED_CONNECTIONS.getBoolean(info)) {
- openStackTrace = new Throwable("Connection was created at this point:");
- }
- finalizeAction = new PgConnectionCleaningAction(lock, openStackTrace, queryExecutor.getCloseAction());
- this.logServerErrorDetail = PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info);
- this.disableColumnSanitiser = PGProperty.DISABLE_COLUMN_SANITISER.getBoolean(info);
- this.convertBooleanToNumeric = PGProperty.CONVERT_BOOLEAN_TO_NUMERIC.getBoolean(info);
+ // Initialize object handling
+ @SuppressWarnings("argument")
+ TypeInfo typeCache = createTypeInfo(this, unknownLength);
+ this.typeCache = typeCache;
+ initObjectTypes(info);
- if (haveMinimumServerVersion(ServerVersion.v8_3)) {
- typeCache.addCoreType("uuid", Oid.UUID, Types.OTHER, "java.util.UUID", Oid.UUID_ARRAY);
- typeCache.addCoreType("xml", Oid.XML, Types.SQLXML, "java.sql.SQLXML", Oid.XML_ARRAY);
- }
+ if (PGProperty.LOG_UNCLOSED_CONNECTIONS.getBoolean(info)) {
+ openStackTrace = new Throwable("Connection was created at this point:");
+ }
+ finalizeAction = new PgConnectionCleaningAction(lock, openStackTrace, queryExecutor.getCloseAction());
+ this.logServerErrorDetail = PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info);
+ this.disableColumnSanitiser = PGProperty.DISABLE_COLUMN_SANITISER.getBoolean(info);
+ this.convertBooleanToNumeric = PGProperty.CONVERT_BOOLEAN_TO_NUMERIC.getBoolean(info);
+
+ if (haveMinimumServerVersion(ServerVersion.v8_3)) {
+ typeCache.addCoreType("uuid", Oid.UUID, Types.OTHER, "java.util.UUID", Oid.UUID_ARRAY);
+ typeCache.addCoreType("xml", Oid.XML, Types.SQLXML, "java.sql.SQLXML", Oid.XML_ARRAY);
+ }
- this.clientInfo = new Properties();
- if (haveMinimumServerVersion(ServerVersion.v9_0)) {
- String appName = PGProperty.APPLICATION_NAME.getOrDefault(info);
- if (appName == null) {
- appName = "";
+ this.clientInfo = new Properties();
+ if (haveMinimumServerVersion(ServerVersion.v9_0)) {
+ String appName = PGProperty.APPLICATION_NAME.getOrDefault(info);
+ if (appName == null) {
+ appName = "";
+ }
+ this.clientInfo.put("ApplicationName", appName);
}
- this.clientInfo.put("ApplicationName", appName);
- }
- fieldMetadataCache = new LruCache<>(
- Math.max(0, PGProperty.DATABASE_METADATA_CACHE_FIELDS.getInt(info)),
- Math.max(0, PGProperty.DATABASE_METADATA_CACHE_FIELDS_MIB.getInt(info) * 1024L * 1024L),
- false);
+ fieldMetadataCache = new LruCache<>(
+ Math.max(0, PGProperty.DATABASE_METADATA_CACHE_FIELDS.getInt(info)),
+ Math.max(0, PGProperty.DATABASE_METADATA_CACHE_FIELDS_MIB.getInt(info) * 1024L * 1024L),
+ false);
- replicationConnection = PGProperty.REPLICATION.getOrDefault(info) != null;
+ replicationConnection = PGProperty.REPLICATION.getOrDefault(info) != null;
- xmlFactoryFactoryClass = PGProperty.XML_FACTORY_FACTORY.getOrDefault(info);
- cleanable = LazyCleanerImpl.getInstance().register(leakHandle, finalizeAction);
+ xmlFactoryFactoryClass = PGProperty.XML_FACTORY_FACTORY.getOrDefault(info);
+ cleanable = LazyCleanerImpl.getInstance().register(leakHandle, finalizeAction);
+ } catch (SQLException | RuntimeException | Error e) {
+ // close() is idempotent (QueryExecutorBase.close checks isClosed), so this is a safe no-op
+ // if a setup step already closed the executor.
+ try {
+ queryExecutor.close();
+ } catch (Exception suppressed) {
+ e.addSuppressed(suppressed);
+ }
+ throw e;
+ }
}
private static ReadOnlyBehavior getReadOnlyBehavior(@Nullable String property) {
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionSetupFailureTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionSetupFailureTest.java
new file mode 100644
index 0000000000..0d3e0f78f7
--- /dev/null
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionSetupFailureTest.java
@@ -0,0 +1,103 @@
+/*
+ * Copyright (c) 2026, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.test.jdbc2;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.postgresql.PGProperty;
+import org.postgresql.test.TestUtil;
+
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Properties;
+
+import javax.net.SocketFactory;
+
+/**
+ * Verifies that the underlying socket is closed when {@code PgConnection} setup fails after the
+ * network connection has already been established. The connection is opened successfully by
+ * {@code ConnectionFactory.openConnection}, then a later setup step throws, so the socket must be
+ * closed by the constructor's cleanup path rather than leaked.
+ */
+class ConnectionSetupFailureTest {
+
+ /**
+ * A {@link SocketFactory} that records every socket it hands out, so the test can assert the
+ * sockets were closed. It is referenced by class name through the {@code socketFactory}
+ * connection property and instantiated by {@code ObjectFactory} via its implicit no-argument
+ * constructor.
+ */
+ public static class RecordingSocketFactory extends SocketFactory {
+ static final List<Socket> SOCKETS = Collections.synchronizedList(new ArrayList<>());
+
+ static void reset() {
+ SOCKETS.clear();
+ }
+
+ private static Socket record(Socket socket) {
+ SOCKETS.add(socket);
+ return socket;
+ }
+
+ @Override
+ public Socket createSocket() {
+ return record(new Socket());
+ }
+
+ @Override
+ public Socket createSocket(String host, int port) throws IOException {
+ return record(new Socket(host, port));
+ }
+
+ @Override
+ public Socket createSocket(String host, int port, InetAddress localHost, int localPort)
+ throws IOException {
+ return record(new Socket(host, port, localHost, localPort));
+ }
+
+ @Override
+ public Socket createSocket(InetAddress host, int port) throws IOException {
+ return record(new Socket(host, port));
+ }
+
+ @Override
+ public Socket createSocket(InetAddress address, int port, InetAddress localAddress,
+ int localPort) throws IOException {
+ return record(new Socket(address, port, localAddress, localPort));
+ }
+ }
+
+ @Test
+ void socketClosedWhenSetupFailsAfterConnect() throws Exception {
+ RecordingSocketFactory.reset();
+
+ Properties props = new Properties();
+ PGProperty.SOCKET_FACTORY.set(props, RecordingSocketFactory.class.getName());
+ // An invalid stringtype value is validated only inside the PgConnection constructor, after the
+ // socket has already been opened, so it triggers the post-openConnection cleanup path.
+ PGProperty.STRING_TYPE.set(props, "this-is-not-a-valid-stringtype");
+
+ SQLException ex = assertThrows(SQLException.class, () -> TestUtil.openDB(props));
+ assertTrue(ex.getMessage().contains("stringtype"),
+ "Expected a stringtype validation failure, but got: " + ex.getMessage());
+
+ assertFalse(RecordingSocketFactory.SOCKETS.isEmpty(),
+ "The connection attempt should have created at least one socket");
+ for (Socket socket : RecordingSocketFactory.SOCKETS) {
+ assertTrue(socket.isClosed(),
+ "Socket should be closed after the failed connection setup: " + socket);
+ }
+ }
+}
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 #4161: fix: close socket when PgConnection setup fails after connect
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