[core] Restore the interrupt status in the two JDBC catalog paths that drop it - #9159
Conversation
…t drop it The jdbc package turns an InterruptedException into an unchecked exception in nine places. Seven re-assert the flag before rethrowing; two do not, so a thread that gets cancelled inside them comes back out looking un-cancelled and every later blocking call on it behaves as if nothing happened. JdbcCatalog:131 the constructor's initializeCatalogTablesIfNeed() call JdbcUtils:677 insertTable Both now follow the shape already used next to them: the single-catch site mirrors JdbcCatalog:1018, and the multi-catch site mirrors JdbcCatalog:1047, which keeps its instanceof guard. No exception type or message changes. JdbcInterruptStatusTest covers both. The catalog constructor needs no mocking: ClientPoolImpl.run waits on LinkedBlockingDeque.pollFirst, whose lockInterruptibly() throws as soon as it sees a thread that already carries the flag, so setting it first drives the real code down its real interrupt path. Both cases fail on master at the interrupt-status assertion.
…ment CI caught this: catalogConstructorKeepsTheInterruptStatus passed locally but failed on Linux with "Expecting code to raise a throwable". The first version set the thread's interrupt flag and relied on the real pool reaching LinkedBlockingDeque.pollFirst, whose lockInterruptibly() throws when the flag is already set. That assumed nothing between the flag and the wait consumes it -- but the constructor opens a real JDBC connection first, and driver initialisation apparently swallows the interrupt on Linux. So no exception, so no assertion. Both cases now stub JdbcClientPool.run to throw InterruptedException outright. For the constructor that means seeding CachedJdbcClientPool's shared cache through its existing @VisibleForTesting clientPools() accessor, the same seam CachedJdbcClientPoolTest already uses, so no real connection is opened at all and there is nothing left to be environment-dependent about. Both still fail against the unfixed catch blocks.
|
CI caught a real defect in my test — pushed 394346d for it.
Both cases now stub Separately, the |
|
+1 |
2 participants
Purpose
The
jdbcpackage converts anInterruptedExceptioninto an unchecked exception in nine places. Seven re-assert the interrupt before rethrowing; two do not. This PR closes those two, so the package stops contradicting itself.JdbcCatalog:131initializeCatalogTablesIfNeed()JdbcUtils:677insertTableJdbcCatalog640, 832, 1018, 1047, 1314 ·JdbcUtils627, 655Why it matters:
LinkedBlockingDeque.pollFirst, which is whatClientPoolImpl.runblocks on, clears the flag when it throws. Whoever catches the resultingRuntimeException— a retry loop, an executor's task wrapper, a catalog-loader that falls back to another catalog — sees a thread that looks like it was never cancelled, and every subsequent blocking call on it behaves accordingly. The two fixed sites are on the catalog-open and table-create paths, so they run on exactly the threads a shutdown is trying to stop.I deliberately kept this to the two deviating sites, matching the shape already used beside each one rather than introducing a new one:
JdbcCatalog:131is a singlecatch (InterruptedException e)→ mirrorsJdbcCatalog:1018JdbcUtils:677is acatch (SQLException | InterruptedException e)→ mirrorsJdbcCatalog:1047, keeping theinstanceofguard so aSQLExceptionis unaffectedNo exception type or message changes, so existing assertions such as
JdbcCatalogTest#testInsertTableUtility'shasMessageContaining("Failed to insert table")still hold.Tests
New
JdbcInterruptStatusTest, one case per fixed site.The constructor case needs no mocking.
ClientPoolImpl.runwaits onLinkedBlockingDeque.pollFirst(10, SECONDS), andlockInterruptibly()throws immediately when the calling thread already carries the flag — so setting the flag and then calling the real constructor drives the real code down its real interrupt path.insertTableis a plain static call, so it takes a mockedJdbcClientPoolwhoserunthrows. Both cases clear the flag in@AfterEachso it cannot leak into later tests on the same thread.Checked against master, both fail — and they fail at the interrupt-status assertion, with the exception type and message assertions already passing, which is what confirms they are exercising the intended path rather than erroring out early:
With the fix:
spotless:check,checkstyle:checkandapache-rat:checkare clean. Java 8 syntax only, perAGENTS.md.No overlap with the open #7475 — its hunks are elsewhere in both files, and it only calls
insertTable.