From e74bf6c18469cd877cc40ae035678929922d6b70 Mon Sep 17 00:00:00 2001 From: daguimu Date: Fri, 27 Mar 2026 12:25:37 +0800 Subject: [PATCH] fix: computeIfAbsent should not update write timestamp for existing entries When computeIfAbsent is called on an existing key, the value is not changed, but recordWrite was called which incorrectly reset the write timestamp. This caused expireAfterWrite caches to behave like expireAfterAccess, as repeated computeIfAbsent calls would extend the entry's lifetime indefinitely. The fix avoids calling recordWrite when the computed value is the same reference as the existing value. Instead, it updates only the access time and re-adds the entry to both access and write queues without resetting the write timestamp. Fixes #3700 --- .../google/common/cache/LocalCacheTest.java | 70 +++++++++++++++++++ .../com/google/common/cache/LocalCache.java | 11 ++- 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/guava-tests/test/com/google/common/cache/LocalCacheTest.java b/guava-tests/test/com/google/common/cache/LocalCacheTest.java index 54e0737fee67..409b402d64a7 100644 --- a/guava-tests/test/com/google/common/cache/LocalCacheTest.java +++ b/guava-tests/test/com/google/common/cache/LocalCacheTest.java @@ -795,6 +795,76 @@ public void testComputeIfAbsent_removalListener() { assertThat(notifications).isEmpty(); } + public void testComputeIfAbsent_existingEntryWriteTimeNotChanged() { + FakeTicker ticker = new FakeTicker(); + LocalCache map = + makeLocalCache( + createCacheBuilder() + .concurrencyLevel(1) + .ticker(ticker) + .expireAfterWrite(10, NANOSECONDS)); + + Object key = new Object(); + Object value = new Object(); + map.put(key, value); + ReferenceEntry entry = map.getEntry(key); + long originalWriteTime = entry.getWriteTime(); + + ticker.advance(5, NANOSECONDS); + + // computeIfAbsent on an existing key should NOT update write time + Object result = map.computeIfAbsent(key, k -> new Object()); + assertThat(result).isSameInstanceAs(value); + assertThat(entry.getWriteTime()).isEqualTo(originalWriteTime); + } + + public void testComputeIfAbsent_existingEntryExpiresAfterWrite() { + FakeTicker ticker = new FakeTicker(); + LocalCache map = + makeLocalCache( + createCacheBuilder() + .concurrencyLevel(1) + .ticker(ticker) + .expireAfterWrite(10, NANOSECONDS)); + + Object key = new Object(); + Object value = new Object(); + map.put(key, value); + + // Repeatedly call computeIfAbsent before expiry + for (int i = 0; i < 5; i++) { + ticker.advance(3, NANOSECONDS); + map.computeIfAbsent(key, k -> new Object()); + } + + // After 15ns total (> 10ns expiry), the entry must be expired + assertThat(map.get(key)).isNull(); + } + + public void testCompute_returningNewValueUpdatesWriteTime() { + FakeTicker ticker = new FakeTicker(); + LocalCache map = + makeLocalCache( + createCacheBuilder() + .concurrencyLevel(1) + .ticker(ticker) + .expireAfterWrite(10, NANOSECONDS)); + + Object key = new Object(); + Object value = new Object(); + map.put(key, value); + ReferenceEntry entry = map.getEntry(key); + long originalWriteTime = entry.getWriteTime(); + + ticker.advance(5, NANOSECONDS); + + // compute() that returns a NEW value SHOULD update write time + Object newValue = new Object(); + Object result = map.compute(key, (k, v) -> newValue); + assertThat(result).isSameInstanceAs(newValue); + assertThat(entry.getWriteTime()).isGreaterThan(originalWriteTime); + } + public void testCopyEntry_computing() { CountDownLatch startSignal = new CountDownLatch(1); CountDownLatch computingSignal = new CountDownLatch(1); diff --git a/guava/src/com/google/common/cache/LocalCache.java b/guava/src/com/google/common/cache/LocalCache.java index c1e0ba26bd32..c33ecc961069 100644 --- a/guava/src/com/google/common/cache/LocalCache.java +++ b/guava/src/com/google/common/cache/LocalCache.java @@ -2275,7 +2275,16 @@ V waitForLoadingValue(ReferenceEntry e, K key, ValueReference valueR if (valueReference != null && newValue == valueReference.get()) { computingValueReference.set(newValue); e.setValueReference(valueReference); - recordWrite(e, 0, now); // no change in weight + // The value was not changed, so avoid resetting the write timestamp, + // which would incorrectly extend the expireAfterWrite deadline (see + // https://github.com/google/guava/issues/3700). + // We still need to re-add to both queues since the entry was removed + // from them above, and update the access time. + if (map.recordsAccess()) { + e.setAccessTime(now); + } + accessQueue.add(e); + writeQueue.add(e); return newValue; } try {