[fix](fe) Fix deadlock in ExternalDatabase.refreshMetaToUninitialized caused by lock inversion with Caffeine cache#65409
[fix](fe) Fix deadlock in ExternalDatabase.refreshMetaToUninitialized caused by lock inversion with Caffeine cache#65409starocean999 wants to merge 3 commits into
Conversation
… caused by lock inversion with Caffeine cache
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review result: request changes.
I found three blocking issues in this PR:
resetMetaToUninitialized()now publishesmetaCache = null, but concurrent readers still dereferencemetaCacheoutside the database monitor aftermakeSureInitialized()returns. This introduces a new NPE race in external table/database metadata resolution.- The added FE test calls
Thread.threadId(), which does not exist under the configured Java 17 compiler target. - The real-path deadlock test coordinates the loader and refresh threads so progress can depend on the 10-second latch timeout rather than deterministic lock-order validation.
Critical checkpoint conclusions:
- Goal/test proof: the intended deadlock fix is not yet proven safely because the production change introduces a new concurrency failure and the new test does not compile as written.
- Scope/focus: the code change is small, but setting
metaCacheto null expands the cache lifecycle state seen by unlocked readers. - Concurrency/lifecycle: this PR is specifically concurrency-sensitive; the old Caffeine lock inversion is addressed in one place, but the replacement cache lifecycle is unsafe for parallel metadata readers.
- Configuration/session/compatibility: no new config was added; session-cache bypass was checked and not raised as a separate issue; Java 17 compatibility is broken by the test API usage.
- Parallel paths:
getTableNullable()andgetTableNamesWithLock()both remain relevant dereference paths after initialization. - Test coverage/results: coverage intent is good, but current tests are blocked by compilation and one concurrency test is timing-dependent.
- Persistence/transactions/data writes/observability: not applicable to this PR beyond external metadata cache invalidation.
User focus: no additional user-provided focus was supplied.
Subagent conclusions: optimizer-rewrite proposed OPT-001 and OPT-002; both were accepted as inline comments MAIN-001 and MAIN-002. tests-session-config marked TEST-001 as a duplicate of OPT-002 and later proposed TEST-002, accepted as MAIN-003. Convergence round 1 found TEST-002, so the ledger/comment set was updated. Convergence round 2 ended with both live subagents returning NO_NEW_VALUABLE_FINDINGS for the final three-comment set.
Validation notes: I could not run the full FE build/test flow because this checkout lacks .worktree_initialized and thirdparty/installed / thirdparty/installed/bin/protoc. I did verify PR head/base metadata via GitHub, existing review context via API, and the Java 17 Thread.threadId() failure with a local javac --release 17 probe.
|
/review |
There was a problem hiding this comment.
Automated follow-up review result for current head 96a5f3da6377133982d9f9dfe735ed503c0887d0: no new inline comments.
I re-reviewed the two changed files and the relevant ExternalDatabase / MetaCache / session-aware catalog paths after the follow-up commit. The three earlier inline comments are not resubmitted: the current head no longer publishes metaCache = null, replaces Thread.threadId() with Thread.getId(), and releases/asserts the real-path test latch before calling resetMetaToUninitialized().
Critical checkpoint conclusions:
- Goal/test proof: the PR still targets the
ExternalDatabase.resetMetaToUninitialized()lock inversion with Caffeine cache loading, and the added test covers the intended deadlock path. Current GitHub CheckStyle is passing. - Scope/focus: the production change is narrow: move
invalidateAll()outside the database monitor and avoid re-enteringmakeSureInitialized()from the cache-loader existence check. - Concurrency/lifecycle: the main cache-field lifecycle concern from the first review is addressed by keeping
metaCachevisible. I checked parallelgetTableNullable(),getTableNamesWithLock(), refresh, and delegated session catalog paths and did not find a new PR-introduced blocker. - Configuration/session/compatibility: no new config or persistence format is added. Delegated Iceberg REST session lookups still bypass the shared table-name cache before reaching the new helper.
- Parallel paths: table-name listing, table lookup by name/id, replay-only cache lookup, and refresh invalidation paths were checked at the relevant call sites.
- Test coverage/results: the new FE unit test is focused on the deadlock regression. I did not run FE tests locally because this checkout lacks
thirdparty/installedandthirdparty/installed/bin/protoc, which the FE instructions treat as a stop-build condition. - Persistence/transactions/data writes/observability: not applicable beyond external metadata cache invalidation; no new logging/metrics requirement found.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite proposed optimizer-rewrite-001; the main review dismissed it with evidence because the internal namesCache invalidation window was already present before this PR and is not introduced by the changed helper. tests-session-config reported no new valuable findings. Convergence round 1 ended with both live subagents returning NO_NEW_VALUABLE_FINDINGS for the same ledger and empty final inline comment set.
Validation notes: local git diff --check on the changed files passed. GitHub PR metadata confirms the reviewed head SHA. Remote PR checks currently show CheckStyle passing; the code-review check is this run.
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review result for current head 9677c93eecc73db8b1e146d3ab6e478de1ed35bb: request changes.
I found one remaining blocking concurrency issue. The latest head addresses the earlier metaCache = null NPE race and the Java 17/test-latch comments, so I did not resubmit those. The remaining issue is that resetMetaToUninitialized() now releases the database monitor before invalidating the old MetaCache, which lets a concurrent reader re-mark the database initialized and read the old cache during the refresh window.
Critical checkpoint conclusions:
- Goal/test proof: the PR targets the
ExternalDatabase.resetMetaToUninitialized()lock inversion with Caffeine cache loading and adds a focused FE unit test, but the production fix still has a concurrency correctness gap. - Scope/focus: the change is narrow, but the cache lifecycle ordering around
initializedandmetaCacheis the key correctness boundary. - Concurrency/lifecycle: this PR is explicitly concurrency-sensitive. Moving
invalidateAll()outside the monitor breaks the deadlock, but it also publishes a reset state before the old cache is invalidated. - Configuration/session/compatibility: no new config or persistence format is added. The previous Java 17
Thread.threadId()issue is fixed on this head, and I did not find a distinct session-cache propagation issue. - Parallel paths:
getTableNullable()andgetTableNamesWithLock()are both affected because they callmakeSureInitialized()and then dereferencemetaCacheoutside the monitor. - Test coverage/results: static checks on the changed files passed (
git diff --checkand line-length scan). I did not run FE tests locally because this checkout lacksthirdparty/installed/bin/protoc, which the FE instructions treat as a stop-build condition. GitHub currently shows COMPILE success and FE UT pending for this head. - Persistence/transactions/data writes/observability: not applicable beyond external metadata cache invalidation.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite reported no new optimizer/planner finding after checking the diff, existing threads, and table-binding paths. tests-session-config reported no new test/session/config finding and noted the same missing local protoc validation limit. After main accepted the single M-1 inline candidate, convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same one-comment final set.
| // Do NOT null metaCache here — callers dereference it outside the monitor | ||
| // after makeSureInitialized() returns (e.g. getTableNullable, getTableNamesWithLock). | ||
| if (cacheToInvalidate != null) { | ||
| cacheToInvalidate.invalidateAll(); |
There was a problem hiding this comment.
Moving the invalidation outside the monitor fixes the Caffeine lock inversion, but this ordering now publishes initialized = false while the old metaCache is still live and not yet invalidated. A query thread can enter makeSureInitialized() in this window, see initialized == false, call buildMetaCache() as a no-op because metaCache is non-null, set initialized = true, and then getTableNullable()/getTableNamesWithLock() can read the old cache before this line invalidates it. That means REFRESH DATABASE can still allow a new reader to plan from stale table metadata during the refresh. Please publish a fresh cache/snapshot under the monitor before invalidating the old one, or otherwise keep readers from reinitializing until the old cache has been invalidated outside the monitor.
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29425 ms |
TPC-DS: Total hot run time: 180529 ms |
ClickBench: Total hot run time: 25.05 s |
2 participants
What problem does this PR solve?
RefreshDatabaseCommand.run() [line 43]
-> RefreshManager.handleRefreshDb() [line 80]
-> refreshDbInternal() [line 120]
-> ExternalDatabase.resetMetaToUninitialized() [line 119]
-> synchronized(this) { [line 126]
metaCache.invalidateAll() [line 132]
-> namesCache.invalidateAll() [MetaCache:148]
-> metaObjCache.invalidateAll() [MetaCache:151]
RefreshTableCommand.run() [line 43]
-> RefreshManager.handleRefreshTable() [line 124]
-> ExternalDatabase.getTableNullable() [line 499]
-> makeSureInitialized() [line 500]
-> MetaCache.getMetaObj() [line 505]
-> synchronized(metaObjCache) { [MetaCache:93]
metaObjCache.get(name) // Caffeine node lock [MetaCache:99]
-> [cache loader from buildMetaCache()] [line 174]
-> buildTableForInit() [line 260]
-> getTableNamesWithLock() [line 271]
-> makeSureInitialized() [line 497]
-> synchronized(this) // DEADLOCK [line 141]
Problem Summary:
ExternalDatabase.resetMetaToUninitialized()callsmetaCache.invalidateAll()while holdingsynchronized(this). This isunnecessary —
MetaCache.invalidateAll()only touches thread-safe Caffeinecaches and a
ConcurrentHashMap, none of which require theExternalDatabasemonitor for correctness. The reference pattern
ExternalCatalog.resetToUninitialized()already calls
onRefreshCache()(which callsinvalidateAll()) outsidesynchronized(this).None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)