Conversation
…ough the redaction of a connect failure reported() rebuilds a connect failure link by link wherever it may carry the credentials of the backend - and on every chain longer than the walk looking for them, which answers "yes" past its 32 links whether the url has a password or not. The rebuild made every link a plain SQLException and put a bare one with no SQLState where its own budget ran out, so a link saying the connection is gone by its type, or standing past the 32nd, read false through isConnectionFailure(): write() neither replayed the attempt nor distrusted the pool over a catalog connect refused the way a shorter chain is replayed. Every rebuilt link now keeps the standard JDBC type it extends, and the link standing for what the budget cut carries the type, SQLState and vendor code of the first link of the rest that says the connection is gone - and says nothing of it where the rest does not, so the cut makes no failure a drop either. Fixes OpenIdentityPlatform#1074
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1074
The problem
CachedConnection.reported()rebuilds a connect failure link by link, andJDBCStorage.write()reads the rebuilt chain throughisConnectionFailure()whenever the catalog connect fails inside a write (newCatalogConnection(): the direct throw, the interrupted wait, andcatalogConnectTimedOut(), whose cause isreported(last)). Two things in that rebuild answered the question with thefalseof a failure that says nothing about the connection:redactedCopy()runs out of its 32 links it putsdroppedTail()in place of the rest - a bareSQLExceptionwith no SQLState, whichsaysTheConnectionIsGone()can never accept. A class 08 link past the 32nd was lost.new SQLException(msg, state, code), so aSQLRecoverableException/SQLNonTransientConnectionException/SQLTransientConnectionExceptionthat says the connection is gone by its type alone - whichsaysTheConnectionIsGone()asks before the SQLState - lost it. The reviewer of [#961] Walk every link of a failure where the verdict decides something #1004 noted this in the thought that became JDBC backend: reported() drops the SQLState of a connection-is-gone link past MAX_CHAIN_LENGTH, so a redacted catalog-connect failure reads false through isConnectionFailure() #1074; the issue text kept only the tail.Two corrections to the issue text, both pinned by the tests below:
holdsCredentials()answers "yes" past its own 32-link walk, so every chain longer than that is rebuilt whether the url has a password or not. The case the issue needs - a deciding link past 32 others - is therefore rebuilt on every deployment; the "only where the failure names the credentials" narrowing does not hold. (I made the same mistake in my reply on [#961] Walk every link of a failure where the verdict decides something #1004.)What was lost:
replayReason()did not replay the attempt as "a connection the database dropped", and the pool was not told - where the same refusal a few links shorter, or on a url without a password, is.The fix
sameKind(): every rebuilt link keeps the standard JDBC type it extends (the three connection types,SQLTimeoutException,SQLTransactionRollbackException,SQLFeatureNotSupportedException, the other standardSQLNonTransient*/SQLTransient*subtypes, elseSQLException). Not the driver's own class, which this cannot be sure of building.droppedTail(rest): walks the links it stands for to their end - by the edgesisConnectionFailure()walks (cause, next exception, suppressed), identity-visited, building nothing - and where one of them says the connection is gone, the tail takes that link's type, SQLState and vendor code. Where none does, the tail stays the bareSQLExceptionit was, so the cut makes no failure a drop either (the concerncatalogConnectTimedOut()'s javadoc spells out about manufacturing class 08).JDBCStorage.saysTheConnectionIsGone()becomes package-private so that the rebuild asks the very question the classifier asks, instead of a second copy of it.Kept as they were: the 32-link budget of the rebuild and of
holdsCredentials(), and the "left out" tail message (testTheTailOfALongChainIsNamedRatherThanDroppedis untouched and green). A memoised rebuild with no budget was considered and not taken: it recurses as deep as the chain, and the budget is also what bounds that.Tests
CachedConnectionTestCase:testALinkThatSaysTheConnectionIsGoneByItsTypeKeepsItThroughRedaction- each of the three connection types, no SQLState, behind a link naming the password.testALinkThatSaysTheConnectionIsGonePastTheBudgetOfARebuildStillSaysSo- 08S01, and a statelessSQLRecoverableException, as the 41st link; url without a password.testALongChainThatSaysNothingOfTheConnectionIsNotMadeToSayItByTheRebuild- guard: 40 plain links stayfalse.CatalogConnectionTestCase(end to end throughnewCatalogConnection()):testARefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo- direct throw.testATimedOutRefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo- 53300 retried to the deadline, 08S01 deep in its next chain.Before the fix: 4 of the 5 red (the guard green, as a guard should be). After:
CachedConnectionTestCase120/120,CatalogConnectionTestCase20/20,JDBCStorageRetryTest113/113,StampConnectionTestCase6/6,Skipped: 0.Mutants, each red on its own case:
SQLExceptionagain…ByItsTypeKeepsItThroughRedaction…PastTheBudgetOfARebuildStillSaysSo+ bothCatalogConnectionTestCasecases…IsNotMadeToSayItByTheRebuild…PastTheBudgetOfARebuildStillSaysSo