From ec514765f713a49ef6d864b53691486f3f8473b4 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 9 Sep 2026 20:12:20 +0300 Subject: [PATCH 1/2] [#932] Hand the bounds of a catalog connect in, so its cases stop setting them for the whole jvm Fixes #932 ## Problem `newCatalogConnection()` read its two bounds from system properties - `org.openidentityplatform.opendj.jdbc.connect.timeout` for one attempt, `org.openidentityplatform.opendj.jdbc.pool.timeout` for the deadline of the whole wait - so a case asserting on either had to set it, and all thirteen cases of `CatalogConnectionTestCase` did: each saved the pair, set what it needed and put it back in a `finally`. For the length of a case the value was the jvm's, and every borrow made anywhere in that jvm reads the same pair on every borrow. `testTheCatalogConnectGivesUpAtTheDeadlineOfABorrow` held `pool.timeout=1` for about a second while it exhausted its deadline. Nothing has failed for it: failsafe runs one class to a fork (`reuseForks=false`, `parallel=none`), and both values are read once at the start of a borrow and turned into a deadline there, so a borrow already running never sees a change. What the window needs to bite is a borrow *begun* inside it on another thread - parallel cases, or a class leaving a borrower running - which is a configuration away and would fail somewhere else entirely. ## Fix The connect takes its bounds as arguments, and the one-arg method every caller of the product uses is the one place that reads the properties into them: Connection newCatalogConnection(long budgetDeadline) // reads the pair Connection newCatalogConnection(long budgetDeadline, // takes it long connectTimeoutSeconds, long poolTimeoutSeconds) Nine cases hand their bounds in and touch no global. Four go on setting the properties, that being the thing they assert - the bound reaching the driver, the default where nothing is set, both turned off, and the deadline capping the attempt - and each holds them for one connect that is answered at once, rather than for a retry that waits. The pair still means the same thing for the pool and for this connect, and is still read on every connect, so an operator can change either on a running server. ## Not in this round `CachedConnectionTestCase` and the container `TestCase` set the same properties; the first clears them in an `@AfterMethod` and drives the pool through its static entry point, the second holds `connect.timeout=2` for one case beside a live backend. Both would need the same seam on `CachedConnection.getConnection()`, which is 25 call sites of test churn for a risk the fork-per-class configuration already covers. No case can tell the two bounds apart by the bound handed to the driver - `attemptSeconds()` takes the lesser of them, which is symmetric - so a delegation that swapped the two would be caught by review rather than by this suite. What the four cases do pin is that each property is read at all. --- .../server/backends/jdbc/JDBCStorage.java | 31 +++- .../jdbc/CatalogConnectionTestCase.java | 143 ++++++------------ 2 files changed, 76 insertions(+), 98 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 328f7dd824..f8aadda53c 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -1453,6 +1453,35 @@ Connection newStampConnection(Dialect dialect) throws SQLException { * CachedConnection#deadlineOf}, so that the shorter of the two is a plain {@code min}. */ Connection newCatalogConnection(long budgetDeadline) throws SQLException { + // the one place this connect reads the two settings, and it reads them on every connect the way + // the pool reads them on every borrow: an operator may change either on a running server + return newCatalogConnection(budgetDeadline, CachedConnection.getConnectTimeoutSeconds(), + CachedConnection.getPoolTimeoutSeconds()); + } + + /** + * The same connect with its two bounds handed in, for the cases of this connect that are not about + * where the bounds come from. + *

+ * Both are read from system properties, here and on every borrow of the pool, so a case pinning + * them by setting the properties holds them for the whole jvm while it runs - and a borrow made + * anywhere else in that window computes a deadline it was never meant to have (#932). The cases + * about the reading itself go on setting the properties, that being the thing they assert; every + * other one hands its values in here, and the jvm hears nothing about it. + *

+ * Not a seam for the product: {@link #newCatalogConnection(long)} is what every caller uses, and + * the pair it reads is the pair a borrow of the pool reads beside it - one property, one meaning, + * whichever of the two is asking. + * + * @param budgetDeadline as {@link #newCatalogConnection(long)} takes it. + * @param connectTimeoutSeconds the bound of one attempt, the way {@link + * CachedConnection#getConnectTimeoutSeconds()} reads it; 0 for an attempt with no bound of + * its own. + * @param poolTimeoutSeconds the deadline of the whole wait, the way {@link + * CachedConnection#getPoolTimeoutSeconds()} reads it; 0 for no deadline of its own. + */ + Connection newCatalogConnection(long budgetDeadline, long connectTimeoutSeconds, long poolTimeoutSeconds) + throws SQLException { // poolKey() rather than the configuration as it stands, for the reason newStampConnection() // gives: this connection is not pooled, but it is a connection to the database of this // storage, and db-directory may be changed on a running backend. Reading it again here would @@ -1461,8 +1490,6 @@ Connection newCatalogConnection(long budgetDeadline) throws SQLException { // rows in one database and tables in another, which is #888 again by another route (#878) final String connectionString=poolKey(); final CachedConnection.ConnectDialect dialect=CachedConnection.ConnectDialect.of(connectionString); - final long connectTimeoutSeconds=CachedConnection.getConnectTimeoutSeconds(); - final long poolTimeoutSeconds=CachedConnection.getPoolTimeoutSeconds(); final long startedAt=System.currentTimeMillis(); final long poolDeadline=CachedConnection.deadlineOf(startedAt, poolTimeoutSeconds); // the deadline of the whole wait, which is the shorter of the pool's own and what is left of diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java index 72d8d92b15..930c6537b5 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java @@ -58,6 +58,13 @@ * so {@code DriverManager} falls through to the probe of this class, while {@code * CachedConnection.ConnectDialect} still reads it as postgres - which is what makes the connect fill * in bounds at all, and what a url of an engine of nobody's would not. + *

+ * The two bounds of the connect are handed in rather than set as system properties, which is where + * it reads them from: both are read on every connect and on every borrow of the pool, so a case + * setting them holds them for the whole jvm while it runs, and a borrow made anywhere else in that + * window computes a deadline it was never meant to have (#932). The four cases about the reading + * itself do set them - that is the thing they assert - and each holds them for one connect that is + * answered at once. */ @SuppressWarnings("javadoc") public class CatalogConnectionTestCase extends DirectoryServerTestCase { @@ -65,10 +72,17 @@ public class CatalogConnectionTestCase extends DirectoryServerTestCase { /** * What a caller with no replay above it hands the connect: the importer is the one such caller in * the product, and every case here that is not about the window itself asks the way it asks, so - * that what it pins is the property and not a window of the test's own. + * that what it pins is the bound and not a window of the test's own. */ private static final long NO_REPLAY_WINDOW = Long.MAX_VALUE; + /** + * The bound of one attempt the cases that are not about that bound hand in: the default of the + * pool, which is what clearing {@link CachedConnection#CONNECT_TIMEOUT_PROPERTY} used to give + * them (#932). + */ + private static final long DEFAULT_BOUND = CachedConnection.DEFAULT_CONNECT_TIMEOUT_SECONDS; + private ProbeDriver probeDriver; @BeforeClass @@ -119,6 +133,8 @@ private static JDBCStorage storageFor(String url) { */ @Test public void testTheCatalogConnectTakesTheConfiguredBound() throws Exception { + // the properties themselves, this being one of the cases about the reading of them: held for a + // connect that is answered at once, where every other case hands its bounds in (#932) final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); System.setProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY, "120"); @@ -141,6 +157,9 @@ public void testTheCatalogConnectTakesTheConfiguredBound() throws Exception { */ @Test public void testTheCatalogConnectIsNeverBoundedPastTheDeadlineOfItsRetry() throws Exception { + // the properties themselves: the deadline of the retry comes from the pool property here rather + // than from a value handed in, which is what pins that half of the reading - the two cannot be + // told apart by the bound alone, the attempt taking the lesser of them (#932) final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); System.setProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY, "120"); @@ -164,22 +183,11 @@ public void testTheCatalogConnectIsNeverBoundedPastTheDeadlineOfItsRetry() throw */ @Test public void testTheCatalogConnectWaitsOutADatabaseTakingNoConnectionForTheMoment() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - // both, since both are read on every connect: an ambient connect bound would change the - // per-attempt bound these cases run under without changing anything they assert on - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "60"); probeDriver.refusal = new SQLException("too many clients already", "53300"); probeDriver.refusalsLeft.set(2); - try { - storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW).close(); - assertEquals(probeDriver.attempts.get(), 3, - "a connect refused for the moment was not retried the way a borrow of the pool retries it"); - } finally { - restore(previous); - restorePool(previousPool); - } + storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 60).close(); + assertEquals(probeDriver.attempts.get(), 3, + "a connect refused for the moment was not retried the way a borrow of the pool retries it"); } /** @@ -194,23 +202,14 @@ public void testTheCatalogConnectWaitsOutADatabaseTakingNoConnectionForTheMoment */ @Test public void testTheCatalogConnectDoesNotRetryAFailureThatWillNotClear() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - // both, since both are read on every connect: an ambient connect bound would change the - // per-attempt bound these cases run under without changing anything they assert on - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "60"); probeDriver.refusal = new SQLException("password authentication failed", "28P01"); probeDriver.refusalsLeft.set(Integer.MAX_VALUE); try { - storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW); + storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 60); fail("a connect that will not clear was retried instead of being reported"); } catch (SQLException expected) { assertEquals(expected.getSQLState(), "28P01", "the failure of the driver was not the one reported"); assertEquals(probeDriver.attempts.get(), 1, "a failure that will not clear was attempted more than once"); - } finally { - restore(previous); - restorePool(previousPool); } } @@ -221,19 +220,13 @@ public void testTheCatalogConnectDoesNotRetryAFailureThatWillNotClear() throws E */ @Test public void testTheCatalogConnectGivesUpAtTheDeadlineOfABorrow() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - // both, since both are read on every connect: an ambient connect bound would change the - // per-attempt bound these cases run under without changing anything they assert on - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "1"); // a vendor code beside the state, since both are carried over: an oracle failure says what it // is in the ORA number and not in the SQLState, so a timeout dropping the code would answer 0 // where the classifier reading it expects the driver's own probeDriver.refusal = new SQLException("the database system is starting up", "57P03", 3113); probeDriver.refusalsLeft.set(Integer.MAX_VALUE); try { - storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW); + storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 1); fail("a connect refused for the whole deadline was not given up on"); } catch (SQLTimeoutException expected) { // the state of the driver's own refusal and not one of this code's making: a manufactured @@ -250,9 +243,6 @@ public void testTheCatalogConnectGivesUpAtTheDeadlineOfABorrow() throws Exceptio // where the property is what ended the wait assertTrue(expected.getMessage().contains(CachedConnection.POOL_TIMEOUT_PROPERTY + "=1s"), "the timeout did not name the bound that ended it: " + expected.getMessage()); - } finally { - restore(previous); - restorePool(previousPool); } } @@ -265,17 +255,13 @@ public void testTheCatalogConnectGivesUpAtTheDeadlineOfABorrow() throws Exceptio */ @Test public void testTheCatalogConnectDoesNotOutlastTheReplayWindowOfItsCaller() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - // a minute, which is the default and six times the window of a write: the property is what - // this connect would wait out if the window did not reach it - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "60"); + // the deadline handed in below is a minute, the default and six times the window of a write: it + // is what this connect would wait out if the window did not reach it probeDriver.refusal = new SQLException("the database system is starting up", "57P03"); probeDriver.refusalsLeft.set(Integer.MAX_VALUE); final long startedAt = System.currentTimeMillis(); try { - storageFor(ProbeDriver.URL).newCatalogConnection(startedAt + 300); + storageFor(ProbeDriver.URL).newCatalogConnection(startedAt + 300, DEFAULT_BOUND, 60); fail("a connect refused past the replay window of its caller was not given up on"); } catch (SQLTimeoutException expected) { final long waited = System.currentTimeMillis() - startedAt; @@ -294,9 +280,6 @@ public void testTheCatalogConnectDoesNotOutlastTheReplayWindowOfItsCaller() thro // an attempt cut to what is left of the window is the connect dying where the pooled one // beside it succeeds - the backend that stops opening assertBoundedAt(probeDriver.lastProperties, CachedConnection.DEFAULT_CONNECT_TIMEOUT_SECONDS); - } finally { - restore(previous); - restorePool(previousPool); } } @@ -308,17 +291,13 @@ public void testTheCatalogConnectDoesNotOutlastTheReplayWindowOfItsCaller() thro */ @Test public void testTheCatalogConnectReportsAnInterruptRatherThanSleepingPastIt() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "60"); probeDriver.refusal = new SQLException("the database system is starting up", "57P03"); probeDriver.refusalsLeft.set(Integer.MAX_VALUE); // raised inside the attempt rather than by another thread racing this one: the first backoff // is a millisecond, and a sleep entered with the flag already up throws at once probeDriver.interruptOnAttempt = true; try { - storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW); + storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 60); fail("a connect interrupted while it waited was not reported at all"); } catch (SQLException expected) { assertFalse(expected instanceof SQLTimeoutException, @@ -336,8 +315,6 @@ public void testTheCatalogConnectReportsAnInterruptRatherThanSleepingPastIt() th } finally { Thread.interrupted(); // cleared here, or every case running after this one on this thread meets it probeDriver.interruptOnAttempt = false; - restore(previous); - restorePool(previousPool); } } @@ -353,10 +330,6 @@ public void testTheCatalogConnectReportsAnInterruptRatherThanSleepingPastIt() th */ @Test public void testAStallOfTheCatalogConnectIsThrottledApartFromTheBorrowsOfThePool() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "60"); // one refusal, slow enough that the connect has been retrying for longer than the guard of // the throttle, and then a connection: what is asserted is the warning, not the failure probeDriver.refusal = new SQLException("the database system is starting up", "57P03"); @@ -365,7 +338,7 @@ public void testAStallOfTheCatalogConnectIsThrottledApartFromTheBorrowsOfThePool try { // a url of its own: the throttle is keyed by url, and a case sharing one with another // would read that one's stamp instead of its own - storageFor(ProbeDriver.STALL_URL).newCatalogConnection(NO_REPLAY_WINDOW).close(); + storageFor(ProbeDriver.STALL_URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 60).close(); final long now = System.currentTimeMillis(); final long longEnoughAgo = now - 2 * CachedConnection.STALL_WARNING_AFTER_MS; // the connect reported its stall: the moment is filed, so the next one inside the interval @@ -378,8 +351,6 @@ public void testAStallOfTheCatalogConnectIsThrottledApartFromTheBorrowsOfThePool "a stall of this connect silenced the borrows of the pool addressing the same database"); } finally { probeDriver.refusalDelayMs = 0; - restore(previous); - restorePool(previousPool); } } @@ -391,6 +362,8 @@ public void testAStallOfTheCatalogConnectIsThrottledApartFromTheBorrowsOfThePool */ @Test public void testTheCatalogConnectTakesTheDefaultWhereNothingIsConfigured() throws Exception { + // the properties themselves, this being one of the cases about the reading of them: held for a + // connect that is answered at once, where every other case hands its bounds in (#932) final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); @@ -416,6 +389,8 @@ public void testTheCatalogConnectTakesTheDefaultWhereNothingIsConfigured() throw */ @Test public void testTheCatalogConnectIsUnboundedWhereTheOperatorTurnedTheBoundOff() throws Exception { + // the properties themselves, this being one of the cases about the reading of them: held for a + // connect that is answered at once, where every other case hands its bounds in (#932) final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); System.setProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY, "0"); @@ -442,22 +417,13 @@ public void testTheCatalogConnectIsUnboundedWhereTheOperatorTurnedTheBoundOff() */ @Test public void testTheCatalogConnectionIsSetUpForItsRows() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); // the read bound is lifted only where the attempt was given one, and the attempt takes the - // shorter of the two properties: a deadline of 0 elsewhere would leave nothing to lift here - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "0"); - try { - final Connection con = storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW); - con.close(); - verify(con).setAutoCommit(false); - verify(con).setTransactionIsolation(Connection.TRANSACTION_READ_COMMITTED); - verify(con).setNetworkTimeout(any(), eq(0)); - } finally { - restore(previous); - restorePool(previousPool); - } + // shorter of the two bounds: a deadline of 0 leaves the per-attempt bound as the only one + final Connection con = storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 0); + con.close(); + verify(con).setAutoCommit(false); + verify(con).setTransactionIsolation(Connection.TRANSACTION_READ_COMMITTED); + verify(con).setNetworkTimeout(any(), eq(0)); } /** @@ -473,23 +439,14 @@ public void testTheCatalogConnectionIsSetUpForItsRows() throws Exception { */ @Test public void testTheCatalogConnectionIsKeptWhereTheReadBoundWillNotComeOff() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "0"); final Connection keeping = mock(Connection.class); doThrow(new SQLFeatureNotSupportedException("no network timeout here")) .when(keeping).setNetworkTimeout(any(), anyInt()); probeDriver.answer = keeping; - try { - final Connection con = storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW); - assertNotNull(con, "a connection whose read bound would not come off was not handed back"); - verify(con).setAutoCommit(false); - verify(con, never()).close(); - } finally { - restore(previous); - restorePool(previousPool); - } + final Connection con = storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 0); + assertNotNull(con, "a connection whose read bound would not come off was not handed back"); + verify(con).setAutoCommit(false); + verify(con, never()).close(); } /** @@ -498,24 +455,18 @@ public void testTheCatalogConnectionIsKeptWhereTheReadBoundWillNotComeOff() thro */ @Test public void testAConnectionWhoseSetUpFailsIsClosed() throws Exception { - final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); - // pinned like every other case of this class: both are read from the system properties on every - // connect, so an ambient value would have this case exercise another path than the one it names - System.clearProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); - System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "0"); + // handed in like every other case that is not about where the bounds come from: read from the + // system properties, an ambient value would have this case exercise another path than it names final Connection failing = mock(Connection.class); doThrow(new SQLException("no transaction here", "08006")).when(failing).setAutoCommit(false); probeDriver.answer = failing; try { - storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW); + storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 0); fail("a connection this backend could not set up was handed to the catalog"); } catch (SQLException expected) { // the failure of the set-up itself, reported to the caller rather than swallowed } finally { probeDriver.answer = null; - restore(previous); - restorePool(previousPool); } verify(failing).close(); } From bbd6dc7e705577afb646b7bd4c006ec64b379919 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 21 Sep 2026 15:04:02 +0300 Subject: [PATCH 2/2] [#932] Pin what the connect was handed, not what the jvm was holding Every case that hands its bounds in handed the default of the pool, which is what the property answers where nothing is set, so three of the four ways to get the seam wrong were invisible to the class: the bound re-read (the driver is handed 30 either way), the deadline re-read (the line prints the argument, so only the sixty seconds it waits give it away), and the pair handed over swapped (the attempt takes the lesser of the two, which is symmetric at the driver). DoesNotOutlastTheReplayWindowOfItsCaller hands in 7 and asserts 7; GivesUpAtTheDeadlineOfABorrow asserts what it waited; and a new case reads the line a wait gives up with, where the deadline is named and a swap names the bound of an attempt instead. Its replay window is handed in already spent, so the two properties it sets stand for one refused connect and no retry waits on them, which is what #932 is about. --- .../jdbc/CatalogConnectionTestCase.java | 75 +++++++++++++++++-- 1 file changed, 70 insertions(+), 5 deletions(-) diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java index 930c6537b5..f582e6ba66 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java @@ -62,9 +62,9 @@ * The two bounds of the connect are handed in rather than set as system properties, which is where * it reads them from: both are read on every connect and on every borrow of the pool, so a case * setting them holds them for the whole jvm while it runs, and a borrow made anywhere else in that - * window computes a deadline it was never meant to have (#932). The four cases about the reading + * window computes a deadline it was never meant to have (#932). The five cases about the reading * itself do set them - that is the thing they assert - and each holds them for one connect that is - * answered at once. + * answered at once, refused or established. */ @SuppressWarnings("javadoc") public class CatalogConnectionTestCase extends DirectoryServerTestCase { @@ -83,6 +83,14 @@ public class CatalogConnectionTestCase extends DirectoryServerTestCase { */ private static final long DEFAULT_BOUND = CachedConnection.DEFAULT_CONNECT_TIMEOUT_SECONDS; + /** + * The bound the one case asserting on what reaches the driver hands in instead: a value neither + * the property nor the default of this code would produce, so that what the case pins is the + * bound it handed in and not what the jvm happens to read. Handed to a connect that reads the + * property instead, it would answer {@link #DEFAULT_BOUND} and the case would say so (#932). + */ + private static final long HANDED_IN_BOUND = 7; + private ProbeDriver probeDriver; @BeforeClass @@ -225,10 +233,17 @@ public void testTheCatalogConnectGivesUpAtTheDeadlineOfABorrow() throws Exceptio // where the classifier reading it expects the driver's own probeDriver.refusal = new SQLException("the database system is starting up", "57P03", 3113); probeDriver.refusalsLeft.set(Integer.MAX_VALUE); + final long startedAt = System.currentTimeMillis(); try { storageFor(ProbeDriver.URL).newCatalogConnection(NO_REPLAY_WINDOW, DEFAULT_BOUND, 1); fail("a connect refused for the whole deadline was not given up on"); } catch (SQLTimeoutException expected) { + // the deadline it waited out is the one it was handed and not one the jvm was holding: a + // connect reading the property in its place gives up a minute from now and says "1s" all + // the same, the line naming what it was handed either way (#932) + final long waited = System.currentTimeMillis() - startedAt; + assertTrue(waited < 30_000, + "the connect waited " + waited + " ms, which is a deadline of the jvm's rather than the one handed in"); // the state of the driver's own refusal and not one of this code's making: a manufactured // 08001 is read by write() as a connection the database dropped, which would replay an // attempt whose pooled connection is healthy and distrust the pool over it @@ -252,6 +267,11 @@ public void testTheCatalogConnectGivesUpAtTheDeadlineOfABorrow() throws Exceptio * the window already spent and is thrown unreplayed - the retry would cost the caller the replay * it had before there was a retry here at all. The shorter of the two bounds is the deadline of * the wait; the bound of one attempt is not taken from it, and this case pins that too. + *

+ * The bound of the attempt is handed in at {@link #HANDED_IN_BOUND} rather than at the default, + * this being the one case that asserts on what reaches the driver from a connect that was handed + * its bounds: a connect reading the property in its place answers the default, which every other + * case hands in, so a value of neither is what tells the two apart (#932). */ @Test public void testTheCatalogConnectDoesNotOutlastTheReplayWindowOfItsCaller() throws Exception { @@ -261,7 +281,7 @@ public void testTheCatalogConnectDoesNotOutlastTheReplayWindowOfItsCaller() thro probeDriver.refusalsLeft.set(Integer.MAX_VALUE); final long startedAt = System.currentTimeMillis(); try { - storageFor(ProbeDriver.URL).newCatalogConnection(startedAt + 300, DEFAULT_BOUND, 60); + storageFor(ProbeDriver.URL).newCatalogConnection(startedAt + 300, HANDED_IN_BOUND, 60); fail("a connect refused past the replay window of its caller was not given up on"); } catch (SQLTimeoutException expected) { final long waited = System.currentTimeMillis() - startedAt; @@ -278,8 +298,10 @@ public void testTheCatalogConnectDoesNotOutlastTheReplayWindowOfItsCaller() thro // and one attempt keeps the bound the operator configured for a login of this database: // the window decides how long it is worth retrying, not how long a login may take, and // an attempt cut to what is left of the window is the connect dying where the pooled one - // beside it succeeds - the backend that stops opening - assertBoundedAt(probeDriver.lastProperties, CachedConnection.DEFAULT_CONNECT_TIMEOUT_SECONDS); + // beside it succeeds - the backend that stops opening. The bound that reaches the driver + // is the one handed in: cut to the window it would be a second, and read off the property + // instead of taken from the argument it would be the default of the pool + assertBoundedAt(probeDriver.lastProperties, HANDED_IN_BOUND); } } @@ -409,6 +431,49 @@ public void testTheCatalogConnectIsUnboundedWhereTheOperatorTurnedTheBoundOff() } } + /** + * The two properties reach the connect the way round they are written: the bound of one attempt + * as the bound of one attempt, and the deadline of the whole wait as the deadline. + *

+ * What reaches the driver cannot tell the two apart - the attempt takes the lesser of the bound + * and what is left of the deadline, which is symmetric in the pair - so the line the connect + * gives up with is the one observable that can: it names the deadline of the wait, and a connect + * handed the pair the other way round names the bound of one attempt there. Left unpinned, a + * swap of the two would reach an operator as a line sending them to raise a property that is + * already higher than the one that ran out. + *

+ * It costs the jvm nothing to ask: the replay window handed in is spent already, so the first + * refusal ends the wait and the properties stand for one connect that is answered at once, the + * way they stand in the cases above (#932). + */ + @Test + public void testTheCatalogConnectReadsTheTwoBoundsTheWayRoundTheyAreWritten() throws Exception { + // the properties themselves, this being one of the cases about the reading of them: two values + // far apart, and neither of them the default of the other, so that the line names which of the + // two it was handed + final String previous = System.getProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY); + final String previousPool = System.getProperty(CachedConnection.POOL_TIMEOUT_PROPERTY); + System.setProperty(CachedConnection.CONNECT_TIMEOUT_PROPERTY, "5"); + System.setProperty(CachedConnection.POOL_TIMEOUT_PROPERTY, "120"); + probeDriver.refusal = new SQLException("the database system is starting up", "57P03"); + probeDriver.refusalsLeft.set(Integer.MAX_VALUE); + try { + // a replay window already spent: the first refusal is past the deadline, so there is no + // backoff to sleep out and no second attempt to make + storageFor(ProbeDriver.URL).newCatalogConnection(System.currentTimeMillis() - 1); + fail("a connect refused with the replay window of its caller already spent was not given up on"); + } catch (SQLTimeoutException expected) { + assertEquals(probeDriver.attempts.get(), 1, + "a connect whose deadline was spent before it began was retried anyway"); + assertTrue(expected.getMessage().contains(CachedConnection.POOL_TIMEOUT_PROPERTY + " is 120s"), + "the line named the bound of one attempt where the deadline of the wait belongs: " + + expected.getMessage()); + } finally { + restore(previous); + restorePool(previousPool); + } + } + /** * What the connection is handed back for: rows of its own, committed where they are written. The * read bound of the login is lifted as soon as the login is through (#872) - it is a bound of the