wenzhenghu commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3600085606
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -222,48 +315,133 @@ private V getWithManualLoad(K key, Function<K, V>
loadFunction) {
return value;
}
+ // Keep the slow miss load under the per-key load lock so concurrent
misses for the same key
+ // are still deduplicated. publishLock(key) only protects the short
publication window.
synchronized (loadLock(key)) {
value = data.asMap().get(key);
if (value != null) {
return value;
}
- long generation = invalidateGeneration.get();
- V loaded = loadAndTrack(key, loadFunction);
- if (generation != invalidateGeneration.get()) {
- return loaded;
+ long generation;
+ // Snapshot generation only after re-checking the cache under the
publication lock so
+ // public mutations cannot slip between the miss observation and
the captured version.
+ synchronized (publishLock(key)) {
+ value = data.asMap().get(key);
+ if (value != null) {
+ return value;
+ }
+ generation = generationOf(key);
}
-
- // Keep null results uncached so manual miss load matches
LoadingCache null-return behavior.
+ V loaded = loadAndTrack(key, loadFunction);
if (loaded == null) {
return null;
}
- // Leave a narrow hook for tests to pause exactly before the cache
put race window.
- beforeManualCachePutForTest(key, loaded);
- data.put(key, loaded);
- if (generation != invalidateGeneration.get()) {
- removeLoadedValue(key, loaded);
+ synchronized (publishLock(key)) {
+ if (generation != generationOf(key)) {
+ return loaded;
+ }
+ // Leave a narrow hook for tests to pause exactly before the
cache put race window.
+ beforeManualCachePutForTest(key, loaded);
+ putLoadedValueWithoutGenerationBump(key, loaded);
+ // Re-check after the put because
invalidateAll()/invalidateIf() may bump stripe generations
+ // outside publishLock while this thread is publishing a
loaded value.
+ if (generation != generationOf(key)) {
+ removeLoadedValueWithoutGenerationBump(key, loaded);
+ }
}
return loaded;
}
}
+ // Keep internal load write-back separate from public mutation so it does
not advance generation.
+ private void putLoadedValueWithoutGenerationBump(K key, V loaded) {
+ data.put(key, loaded);
+ }
+
// Remove only the value loaded by the current request and keep newer
replacements intact.
- private void removeLoadedValue(K key, V loaded) {
+ private void removeLoadedValueWithoutGenerationBump(K key, V loaded) {
data.asMap().computeIfPresent(key, (ignored, currentValue) ->
currentValue == loaded ? null : currentValue);
}
+ private CacheLoader<K, V> newCacheLoader() {
+ return new CacheLoader<K, V>() {
+ @Override
+ public V load(K key) {
+ return loadFromDefaultLoader(key);
+ }
+
+ @Override
+ public CompletableFuture<V> asyncReload(K key, V oldValue,
Executor executor) {
+ long generation = generationOf(key);
+ CompletableFuture<V> result = new CompletableFuture<>();
+ CompletableFuture.supplyAsync(() ->
loadFromDefaultLoader(key), executor)
+ .whenComplete((loaded, error) -> {
+ if (error != null) {
+ result.completeExceptionally(error);
+ return;
+ }
+ synchronized (publishLock(key)) {
+ if (generation == generationOf(key)) {
+ result.complete(loaded);
Review Comment:
Thanks for identifying this race. We agree that it is theoretically possible
when an asynchronous refresh, capacity eviction, and bulk invalidation overlap
in a narrow publication window.
We do not plan to address it in this PR. This behavior already exists in the
baseline Caffeine refresh semantics and is not introduced or worsened by this
refactor. The generation-aware refresh added here prevents stale publication in
the ordinary invalidation paths, while this remaining case requires the key to
be evicted during refresh and bulk invalidation to overlap with refresh
completion.
The impact is limited to a temporarily stale local cache entry. It does not
affect persisted metadata or data correctness, and the entry is eventually
corrected by TTL, a subsequent refresh, or another explicit invalidation. Fully
linearizing this case would require broader publication coordination and
introduce additional locking complexity into the cache path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
