fix: isValid() should not wait longer than network timeout

pgjdbc · Jan 26, 2021 · 0dbc607 · 0dbc607
1 parent 8f773a1
commit 0dbc607
Show file tree

Hide file tree

Showing 4 changed files with 96 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 ### Fixed
 - Fix "Required class information missing" when old org.jboss:jandex parses pgjdbc classes [issue 2008][https://github.com/pgjdbc/pgjdbc/issues/2008]
 - Fix PGCopyInputStream returning the last row twice when reading with CopyOut API [issue 2016][https://github.com/pgjdbc/pgjdbc/issues/2016]
+- Fix Connnection.isValid() to not wait longer than existing network timeout [PR #2040](https://github.com/pgjdbc/pgjdbc/pull/2040)
 
 ## [42.2.18]
 ### Fixed

diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
@@ -1408,10 +1408,17 @@ public boolean isValid(int timeout) throws SQLException {
     if (isClosed()) {
       return false;
     }
+    boolean changedNetworkTimeout = false;
     try {
-      int savedNetworkTimeOut = getNetworkTimeout();
+      int oldNetworkTimeout = getNetworkTimeout();
+      int newNetworkTimeout = (int) Math.min(timeout * 1000L, Integer.MAX_VALUE);
       try {
-        setNetworkTimeout(null, timeout * 1000);
+        // change network timeout only if the new value is less than the current
+        // (zero means infinite timeout)
+        if (newNetworkTimeout != 0 && (oldNetworkTimeout == 0 || newNetworkTimeout < oldNetworkTimeout)) {
+          changedNetworkTimeout = true;
+          setNetworkTimeout(null, newNetworkTimeout);
+        }
         if (replicationConnection) {
           Statement statement = createStatement();
           statement.execute("IDENTIFY_SYSTEM");
@@ -1425,7 +1432,9 @@ public boolean isValid(int timeout) throws SQLException {
         }
         return true;
       } finally {
-        setNetworkTimeout(null, savedNetworkTimeOut);
+        if (changedNetworkTimeout) {
+          setNetworkTimeout(null, oldNetworkTimeout);
+        }
       }
     } catch (SQLException e) {
       if (PSQLState.IN_FAILED_SQL_TRANSACTION.getState().equals(e.getSQLState())) {

diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc4/ConnectionValidTimeoutTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc4/ConnectionValidTimeoutTest.java
@@ -0,0 +1,82 @@
+/*
+ * Copyright (c) 2021, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.test.jdbc4;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.postgresql.test.TestUtil;
+import org.postgresql.test.util.StrangeProxyServer;
+import org.postgresql.test.util.rules.annotation.HaveMinimalServerVersion;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.sql.Connection;
+import java.util.Arrays;
+import java.util.Properties;
+import java.util.concurrent.TimeUnit;
+
+@RunWith(Parameterized.class)
+@HaveMinimalServerVersion("9.4")
+public class ConnectionValidTimeoutTest {
+
+  @Rule
+  public Timeout timeout = new Timeout(30, TimeUnit.SECONDS);
+
+  @Parameterized.Parameter(0)
+  public int networkTimeoutMillis;
+  @Parameterized.Parameter(1)
+  public int validationTimeoutSeconds;
+  @Parameterized.Parameter(2)
+  public int expectedMaxValidationTimeMillis;
+
+  @Parameterized.Parameters(name = "networkTimeoutMillis={0}, validationTimeoutSeconds={1}, expectedMaxValidationTimeMillis={2}")
+  public static Iterable<Object[]> data() {
+    return Arrays.asList(new Object[][]{
+      {500, 1, 600},
+      {1500, 1, 1100},
+      {0, 1, 1100},
+      {500, 0, 600},
+    });
+  }
+
+  @Test
+  public void testIsValidRespectsSmallerTimeout() throws Exception {
+    try (StrangeProxyServer proxyServer = new StrangeProxyServer(TestUtil.getServer(), TestUtil.getPort())) {
+      final Properties props = new Properties();
+      props.setProperty(TestUtil.SERVER_HOST_PORT_PROP, String.format("%s:%s", "localhost", proxyServer.getServerPort()));
+      try (Connection conn = TestUtil.openDB(props)) {
+        assertTrue("Connection through proxy should be valid", conn.isValid(validationTimeoutSeconds));
+
+        conn.setNetworkTimeout(null, networkTimeoutMillis);
+        assertTrue("Connection through proxy should still be valid", conn.isValid(validationTimeoutSeconds));
+
+        proxyServer.stopForwardingOlderClients();
+
+        long start = System.currentTimeMillis();
+        boolean result = conn.isValid(validationTimeoutSeconds);
+        long elapsed = System.currentTimeMillis() - start;
+
+        assertFalse("Broken connection should not be valid", result);
+
+        assertTrue(String.format(
+            "Connection validation should not take longer than %d ms"
+                + " when network timeout is %d ms and validation timeout is %d s"
+                + " (actual result: %d ms)",
+            expectedMaxValidationTimeMillis,
+            networkTimeoutMillis,
+            validationTimeoutSeconds,
+            elapsed),
+            elapsed <= expectedMaxValidationTimeMillis
+        );
+      }
+    }
+  }
+}
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc4/Jdbc4TestSuite.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc4/Jdbc4TestSuite.java
@@ -19,6 +19,7 @@
     BlobTest.class,
     CharacterStreamTest.class,
     ClientInfoTest.class,
+    ConnectionValidTimeoutTest.class,
     DatabaseMetaDataHideUnprivilegedObjectsTest.class,
     DatabaseMetaDataTest.class,
     IsValidTest.class,