diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java index 814304a7ca..3a0f01f063 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java @@ -258,7 +258,8 @@ static long nextBackoffMs(long backoffMs) { * How many links of the cause and getNextException() chains of a failure the two walks that * have a reason to stop look at: {@link #holdsCredentials} answers "yes" past it, since the * cost of the other answer is the password of the backend in the log, and - * {@link #redactedCopy} rebuilds that far and names the rest in one link. The walk whose verdict + * {@link #redactedCopy} rebuilds that far and names the rest in one link - carrying, where the + * rest says the connection is gone, what says it (#1074). The walk whose verdict * decides something, {@link #isWorthRetrying}, is not one of them: a count does not leave that * question unanswered, it answers it with the verdict of a failure that carries nothing * (issue #1076), and the visited set of every walk here terminates it on its own. @@ -2135,8 +2136,11 @@ static String stallMessage(String connectionString, int attempts, long waitedMs, * prints a failure prints the causes along with it - a debug build of * stackTraceToSingleLineString walks them, the config manager traces them, and * RootContainer.open() makes the message of the cause the message of what it throws - so a - * link left as it stands would carry the password past the wrapper. The SQLState and the - * vendor code of every link survive it: they are what tells a caller what happened. + * link left as it stands would carry the password past the wrapper. The SQLState, the vendor + * code and the standard JDBC type of every link survive it: they are what tells a caller what + * happened, and JDBCStorage.write() asks the type before the state whether the connection is + * gone. Past the budget of the rebuild the one link standing for the rest says that much of it + * where the rest says so (#1074). *

* The whole chain is the causes, the further exceptions of {@code getNextException()} and what * was suppressed on each of them - the last being where this class puts the failure of a close @@ -2223,40 +2227,126 @@ private static void enqueue(Deque pending, Set visited, Th private static SQLException redactedCopy(SQLException e, String connectionString, int[] budget) { budget[0]--; final SQLException copy = - new SQLException(redact(e.getMessage(), connectionString), e.getSQLState(), e.getErrorCode()); + sameKind(e, redact(e.getMessage(), connectionString), e.getSQLState(), e.getErrorCode()); copy.setStackTrace(e.getStackTrace()); if (e.getNextException() != null) { copy.setNextException(budget[0] > 0 - ? redactedCopy(e.getNextException(), connectionString, budget) : droppedTail()); + ? redactedCopy(e.getNextException(), connectionString, budget) + : droppedTail(Collections.singletonList(e.getNextException()))); } if (e.getCause() != null) { - copy.initCause(budget[0] > 0 ? redactedLink(e.getCause(), connectionString, budget) : droppedTail()); + copy.initCause(budget[0] > 0 ? redactedLink(e.getCause(), connectionString, budget) + : droppedTail(Collections.singletonList(e.getCause()))); } copySuppressed(e, copy, connectionString, budget); return copy; } + /** + * A new exception of the kind JDBC names the given one as. Not the class of the driver itself, + * which this cannot be sure of building, but the standard type it extends: that type is what + * the JDBC contract gives a driver to say what happened beside the SQLState, and JDBCStorage + * reads it first - a SQLRecoverableException says the connection is gone whatever state it + * carries, and a copy made as a plain SQLException says nothing of the kind (#1074). + */ + private static SQLException sameKind(SQLException e, String message, String sqlState, int vendorCode) { + if (e instanceof SQLRecoverableException) { + return new SQLRecoverableException(message, sqlState, vendorCode); + } + if (e instanceof SQLNonTransientConnectionException) { + return new SQLNonTransientConnectionException(message, sqlState, vendorCode); + } + if (e instanceof SQLTransientConnectionException) { + return new SQLTransientConnectionException(message, sqlState, vendorCode); + } + if (e instanceof SQLTimeoutException) { + return new SQLTimeoutException(message, sqlState, vendorCode); + } + if (e instanceof SQLTransactionRollbackException) { + return new SQLTransactionRollbackException(message, sqlState, vendorCode); + } + if (e instanceof SQLFeatureNotSupportedException) { + return new SQLFeatureNotSupportedException(message, sqlState, vendorCode); + } + if (e instanceof SQLIntegrityConstraintViolationException) { + return new SQLIntegrityConstraintViolationException(message, sqlState, vendorCode); + } + if (e instanceof SQLInvalidAuthorizationSpecException) { + return new SQLInvalidAuthorizationSpecException(message, sqlState, vendorCode); + } + if (e instanceof SQLSyntaxErrorException) { + return new SQLSyntaxErrorException(message, sqlState, vendorCode); + } + if (e instanceof SQLDataException) { + return new SQLDataException(message, sqlState, vendorCode); + } + if (e instanceof SQLTransientException) { + return new SQLTransientException(message, sqlState, vendorCode); + } + if (e instanceof SQLNonTransientException) { + return new SQLNonTransientException(message, sqlState, vendorCode); + } + return new SQLException(message, sqlState, vendorCode); + } + // The suppressed links of a failure are rebuilt with the rest of it and counted against the // same budget. They are not decoration here: the failure of a close that could not be made // rides on the failure being unwound (establish(), #929), and a rebuild that dropped them // would lose it at the one deployment whose log is redacted - which is the deployment whose // url has a password in it, the log that is worth reading. private static void copySuppressed(Throwable from, Throwable to, String connectionString, int[] budget) { - for (final Throwable suppressed : from.getSuppressed()) { + final Throwable[] suppressed = from.getSuppressed(); + for (int i = 0; i < suppressed.length; i++) { if (budget[0] <= 0) { - to.addSuppressed(droppedTail()); + to.addSuppressed(droppedTail(Arrays.asList(suppressed).subList(i, suppressed.length))); return; } - to.addSuppressed(redactedLink(suppressed, connectionString, budget)); + to.addSuppressed(redactedLink(suppressed[i], connectionString, budget)); } } // What stands where the budget ran out. Without it the same failure logs its root cause when // the url of the backend has no password in it and loses it without a word when it has, which // is a report of a connect nobody can read against a report of one they can. - private static SQLException droppedTail() { - return new SQLException("the rest of this failure was left out: a chain of more than " - + MAX_CHAIN_LENGTH + " links is rebuilt only that far"); + // It also says what the rest says about the connection, where the rest says it is gone: that + // is a verdict JDBCStorage.write() reads off every link of the failure - it replays the attempt + // and distrusts the pool on it - and a tail saying nothing would answer it with the "no" of a + // failure that carries nothing (#1074, the #961 thesis on the budget of this rebuild). Only + // that, and only where the rest does say it, so that the cut makes no failure say it either. + private static SQLException droppedTail(List rest) { + final String message = "the rest of this failure was left out: a chain of more than " + + MAX_CHAIN_LENGTH + " links is rebuilt only that far"; + final SQLException gone = firstLinkSayingTheConnectionIsGone(rest); + return gone == null + ? new SQLException(message) + : sameKind(gone, message + "; a link of it says the connection is gone", gone.getSQLState(), + gone.getErrorCode()); + } + + // Every link of the rest, by the edges JDBCStorage.isConnectionFailure() walks - the causes, the + // further exceptions and what was suppressed - and to its end: the visited set terminates it, and + // it builds nothing, so what it costs is a walk of the links the driver has already allocated. + private static SQLException firstLinkSayingTheConnectionIsGone(List rest) { + final Deque pending = new ArrayDeque<>(); + final Set visited = Collections.newSetFromMap(new IdentityHashMap()); + for (final Throwable t : rest) { + enqueue(pending, visited, t); + } + while (!pending.isEmpty()) { + final Throwable t = pending.poll(); + if (t instanceof SQLException) { + final SQLException sql = (SQLException) t; + if (JDBCStorage.saysTheConnectionIsGone(sql)) { + return sql; + } + enqueue(pending, visited, sql.getNextException()); + } + enqueue(pending, visited, t.getCause()); + for (final Throwable suppressed : t.getSuppressed()) { + enqueue(pending, visited, suppressed); + } + } + return null; } // A link that is no SQLException keeps its class name in the message: its type is not one this @@ -2270,7 +2360,8 @@ private static Throwable redactedLink(Throwable t, String connectionString, int[ + (t.getMessage() == null ? "" : ": " + redact(t.getMessage(), connectionString))); copy.setStackTrace(t.getStackTrace()); if (t.getCause() != null) { - copy.initCause(budget[0] > 0 ? redactedLink(t.getCause(), connectionString, budget) : droppedTail()); + copy.initCause(budget[0] > 0 ? redactedLink(t.getCause(), connectionString, budget) + : droppedTail(Collections.singletonList(t.getCause()))); } copySuppressed(t, copy, connectionString, budget); return copy; 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 e38d3cfb7f..de316f162c 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 @@ -4924,8 +4924,12 @@ private static void walkLinks(Throwable failure, boolean withTheRelease, int lin * a driver that raises one of them has said so whatever state it filled in. Oracle reports ORA-03113, ORA-00028 * and ORA-01089 as {@link SQLRecoverableException} and happens to map them to 08006 as well; the type is what * makes that robust rather than lucky. + *

+ * Package-private for the one other reader it has, the redaction of a connect failure in {@link + * CachedConnection}: what that rebuild leaves in place of a chain past its budget has to say what this reads + * off the links it cuts (#1074), and a second copy of the question would drift from this one. */ - private static boolean saysTheConnectionIsGone(SQLException e) { + static boolean saysTheConnectionIsGone(SQLException e) { if (e instanceof SQLRecoverableException || e instanceof SQLNonTransientConnectionException || e instanceof SQLTransientConnectionException) { return true; diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java index ce66a7e434..bb233814f5 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java @@ -40,7 +40,10 @@ import java.sql.DriverPropertyInfo; import java.sql.PreparedStatement; import java.sql.SQLException; +import java.sql.SQLNonTransientConnectionException; +import java.sql.SQLRecoverableException; import java.sql.SQLTimeoutException; +import java.sql.SQLTransientConnectionException; import java.util.ArrayDeque; import java.util.Collections; import java.util.Deque; @@ -941,6 +944,72 @@ public void testTheTailOfALongChainIsNamedRatherThanDropped() throws Exception { "the tail a rebuild had no budget for has to say so: " + last.getMessage()); } + /** + * A link that says the connection is gone by its type alone still says so once it is rebuilt + * (#1074). The type is what the JDBC contract gives a driver to say it - write() asks it before + * the SQLState - so a copy made as a plain SQLException reads as a failure that says nothing of + * the connection, and only at the deployment whose url has a password in it. + */ + @Test(timeOut = 60000) + public void testALinkThatSaysTheConnectionIsGoneByItsTypeKeepsItThroughRedaction() throws Exception { + final String url = "jdbc:postgresql://opendj:S3cretOfTheBackend@127.0.0.1:5432/opendj"; + for (final SQLException gone : new SQLException[] { new SQLRecoverableException("io error"), + new SQLNonTransientConnectionException("closed"), new SQLTransientConnectionException("reset") }) { + final SQLException failure = new SQLException("login to " + url + " failed", "S0001", 18456); + failure.setNextException(gone); + + final SQLException reported = CachedConnection.reported(failure, url); + + assertNoCredentials(reported); + assertTrue(JDBCStorage.isConnectionFailure(reported), + "a rebuilt " + gone.getClass().getSimpleName() + " no longer says the connection is gone"); + } + } + + /** + * ... and so does one standing past the budget of the rebuild, whatever it says it by: the link + * that stands for the rest of the chain there is all that the classification of write() gets to + * read of it. The url has no password, and the chain is rebuilt all the same - it is longer than + * the walk looking for credentials, which answers "yes" past it. + */ + @Test(timeOut = 60000) + public void testALinkThatSaysTheConnectionIsGonePastTheBudgetOfARebuildStillSaysSo() throws Exception { + final String url = "jdbc:postgresql://127.0.0.1:5432/opendj"; + for (final SQLException gone : new SQLException[] { new SQLException("connection reset", "08S01", 10054), + new SQLRecoverableException("io error") }) { + final SQLException failure = plainChain(40); + failure.setNextException(gone); + + final SQLException reported = CachedConnection.reported(failure, url); + + assertTrue(reported != failure, "a chain this long is expected to be rebuilt"); + assertTrue(JDBCStorage.isConnectionFailure(reported), + gone + " past the budget of the rebuild no longer says the connection is gone"); + } + } + + /** + * The link standing for the rest says only what the rest says: a chain that says nothing of the + * connection is not made to say it is gone by being cut. That would replay the write and distrust + * the pool over a database that refused a connection for a reason of its own. + */ + @Test(timeOut = 60000) + public void testALongChainThatSaysNothingOfTheConnectionIsNotMadeToSayItByTheRebuild() throws Exception { + final SQLException reported = CachedConnection.reported(plainChain(40), "jdbc:postgresql://127.0.0.1:5432/opendj"); + + assertFalse(JDBCStorage.isConnectionFailure(reported), + "the rebuild made a failure that says nothing of the connection say it is gone"); + } + + /** That many plain links of the next exception chain, none of which says the connection is gone. */ + private static SQLException plainChain(int links) { + final SQLException head = new SQLException("error 0", "S0001", 1); + for (int i = 1; i < links; i++) { + head.setNextException(new SQLException("error " + i, "S0001", 1)); + } + return head; + } + /** * A credential named by a suppressed link alone is redacted like any other. The close of a * connection whose set-up failed rides there (establish(), #929) and an interrupt that ended a 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 53404b31bc..17af3506b1 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 @@ -685,6 +685,64 @@ public void testAConnectionWhoseSetUpFailsIsClosed() throws Exception { verify(failing).close(); } + /** + * A refusal that says the connection is gone reaches {@code write()} saying so, however deep in its + * chain the driver put the link that says it (#1074). The failure leaves this connect redacted, and + * a chain longer than the walk looking for credentials is rebuilt whether or not anything in it names + * them - this url has no password at all - so what the rebuild leaves past its budget is what {@code + * isConnectionFailure()} reads: the attempt is replayed where the same refusal a few links shorter is. + */ + @Test + public void testARefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo() throws Exception { + final SQLException refusal = deepChain(40, new SQLException("connection reset", "08S01", 10054)); + probeDriver.refusal = refusal; + probeDriver.refusalsLeft.set(Integer.MAX_VALUE); + try { + 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) { + assertTrue(JDBCStorage.isConnectionFailure(refusal), "the refusal of the driver says the connection is gone"); + assertTrue(JDBCStorage.isConnectionFailure(expected), + "the refusal as it left the connect lost what said the connection is gone: " + expected); + } + } + + /** + * ... and so does the timeout a refusal worth waiting out ends in, whose cause is that refusal + * redacted the same way: the retry changes no classification. + */ + @Test + public void testATimedOutRefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo() throws Exception { + final SQLException refusal = new SQLException("too many clients already", "53300"); + refusal.setNextException(deepChain(40, new SQLException("connection reset", "08S01", 10054))); + probeDriver.refusal = refusal; + probeDriver.refusalsLeft.set(Integer.MAX_VALUE); + 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) { + assertTrue(JDBCStorage.isConnectionFailure(refusal), "the refusal of the driver says the connection is gone"); + assertTrue(JDBCStorage.isConnectionFailure(expected), + "the timeout lost what its last refusal said about the connection: " + expected); + } + } + + /** + * A driver's failure the way mssql-jdbc chains every error of a message it received: that many + * plain links of the next exception chain, and the given one last. + */ + private static SQLException deepChain(int links, SQLException last) { + final SQLException head = new SQLException("error 0 of the login", "S0001", 1); + SQLException tail = head; + for (int i = 1; i < links; i++) { + final SQLException next = new SQLException("error " + i + " of the login", "S0001", 1); + tail.setNextException(next); + tail = next; + } + tail.setNextException(last); + return head; + } + /** What the dialect of this url declares, at the given number of seconds. */ private static void assertBoundedAt(Properties handed, long seconds) { assertNotNull(handed, "no properties were handed to the driver at all");