Skip to content

Core: Fix race condition in CachingCatalog loadTable (#17338) - #17375

Open
fightBoxing wants to merge 1 commit into
apache:mainfrom
fightBoxing:fix/caching-catalog-race-condition-17338
Open

Core: Fix race condition in CachingCatalog loadTable (#17338)#17375
fightBoxing wants to merge 1 commit into
apache:mainfrom
fightBoxing:fix/caching-catalog-race-condition-17338

Conversation

@fightBoxing

Copy link
Copy Markdown

Summary

Fix the race condition in CachingCatalog.loadTable() as reported in #17338,其中 Cache.putCache.invalidate / Cache.invalidateAll 之间存在竞态条件,可能导致陈旧(stale)的表引用被重新放回缓存。

Race Condition

Thread A: 检查缓存 → 空,开始加载新值(此时尚未 stale)
Thread B: 修改外部状态(A 计算的值变为 stale),调用 invalidate()
Thread A: 将 stale 值 put 回缓存  ← ❌ 陈旧值残留在缓存中直到 TTL 过期

这在启用 OpenLineage(后台线程调用 loadTable())与写入操作并发时尤其容易被触发。

Fix

loadTable() 中原有的三步非原子操作替换为单次 tableCache.asMap().compute() 原子调用:

// ❌ 旧代码:getIfPresent + get + put 三步之间存在竞态窗口
Table table = tableCache.get(canonicalized, catalog::loadTable);
if (table instanceof BaseMetadataTable) {
    tableCache.put(canonicalized, metadataTable);  // 与 invalidate 竞争
}

// ✅ 新代码:asMap().compute() 全原子操作
return tableCache.asMap().compute(canonicalized, (key, existing) -> {
    if (existing != null) return existing;
    Table table = catalog.loadTable(key);
    // ... metadata table 处理也在 compute 内部完成
});

ConcurrentHashMap(Caffeine 的底层实现)保证同一 key 上的 computeremove(即 invalidate)是串行化的,从而消除了竞态窗口。

Test

新增回归测试 testConcurrentLoadAndInvalidateDoesNotResurrectStaleEntry

  • 使用 CountDownLatch 精确同步 loadTableinvalidateTable 同时启动
  • 反复 200 次并发碰撞
  • 验证每次碰撞后 loadTable 始终返回有效引用

所有原有 TestCachingCatalog 测试(13 个用例)全部通过,无回归。

Files Changed

  • core/src/main/java/org/apache/iceberg/CachingCatalog.java — 重构 loadTable() 使用原子 compute
  • core/src/test/java/org/apache/iceberg/hadoop/TestCachingCatalog.java — 新增并发回归测试

Closes #17338.

Replace non-atomic getIfPresent + get + put sequence in loadTable() with
asMap().compute() to eliminate race condition between concurrent loadTable()
and invalidateTable() that could resurrect stale entries in the cache.

The race: Thread A checks cache (miss) and starts loading; Thread B
invalidates the cache; Thread A finishes loading and put-s a stale value.
With asMap().compute(), ConcurrentHashMap serializes compute and remove
on the same key, preventing the stale put.

Also add regression test testConcurrentLoadAndInvalidateDoesNotResurrectStaleEntry
that stress-tests 200 iterations of concurrent loadTable + invalidateTable.
Table originTable =
tableCache
.asMap()
.computeIfAbsent(originTableIdentifier, catalog::loadTable);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metadata branch performs this nested call from inside the outer compute(canonicalized, …) remapping function, mutating the same cache during its own computation. This is the anti-pattern PR #3801 deliberately removed after production deadlock #3791; both Caffeine's asMap() view and the CHM it wraps document that the mapping function must not update the map during computation. Depending on interleaving and key hashing, it can deadlock or throw a "Recursive update" IllegalStateException, and the still-synchronous RemovalListener firing invalidateAll(...) during eviction compounds the liveness surface. The branch is reachable by any ordinary metadata-table load (e.g. table.snapshots, table.files), so this is a common path, not an edge case. Merging as-is risks regressing #3791.

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comment above, the approach might need to be redesigned a bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race Condition in CachingCatalog

2 participants