Skip to content

Commit

Permalink
Fix log message when detecting a broken key equality (fixes #900)
Browse files Browse the repository at this point in the history
Previously the message used slf4j's format which differs from
System.Logger's. This would be broken anyway due to SLF4J-529 as it
uses String.format. Instead the message is formatted explicitly and the
tests assert the contents.
  • Loading branch information
ben-manes committed Apr 6, 2023
1 parent 00f5862 commit 1eb57ac
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@
import static com.github.benmanes.caffeine.cache.Caffeine.calculateHashMapCapacity;
import static com.github.benmanes.caffeine.cache.Caffeine.ceilingPowerOfTwo;
import static com.github.benmanes.caffeine.cache.Caffeine.requireArgument;
import static com.github.benmanes.caffeine.cache.Caffeine.requireState;
import static com.github.benmanes.caffeine.cache.Caffeine.saturatedToNanos;
import static com.github.benmanes.caffeine.cache.LocalLoadingCache.newBulkMappingFunction;
import static com.github.benmanes.caffeine.cache.LocalLoadingCache.newMappingFunction;
import static com.github.benmanes.caffeine.cache.Node.PROBATION;
import static com.github.benmanes.caffeine.cache.Node.PROTECTED;
import static com.github.benmanes.caffeine.cache.Node.WINDOW;
import static java.util.Locale.US;
import static java.util.Objects.requireNonNull;
import static java.util.Spliterator.DISTINCT;
import static java.util.Spliterator.IMMUTABLE;
Expand Down Expand Up @@ -289,21 +289,29 @@ protected BoundedLocalCache(Caffeine<K, V> builder,
}

/** Ensures that the node is alive during the map operation. */
static void requireIsAlive(Object key, Node<?, ?> node) {
requireState(node.isAlive(), "An invalid state was detected that occurs if the key's equals "
+ "or hashCode was modified while it resided in the cache. This violation of the Map "
+ "contract can lead to non-deterministic behavior (key: %s).", key);
void requireIsAlive(Object key, Node<?, ?> node) {
if (!node.isAlive()) {
throw new IllegalStateException(brokenEqualityMessage(key, node));
}
}

/** Logs if the node cannot be found in the map but is still alive. */
static void logIfAlive(Node<?, ?> node) {
void logIfAlive(Node<?, ?> node) {
if (node.isAlive()) {
logger.log(Level.ERROR, "An invalid state was detected that occurs if the key's equals or "
+ "hashCode was modified while it resided in the cache. This violation of the Map "
+ "contract can lead to non-deterministic behavior (key: {}).", node.getKeyReference());
String message = brokenEqualityMessage(node.getKeyReference(), node);
logger.log(Level.ERROR, message, new IllegalStateException());
}
}

/** Returns the formatted broken equality error message. */
String brokenEqualityMessage(Object key, Node<?, ?> node) {
return String.format(US, "An invalid state was detected that occurs if the key's equals or "
+ "hashCode was modified while it resided in the cache. This violation of the Map "
+ "contract can lead to non-deterministic behavior (key: %s, key type: %s, "
+ "node type: %s, cache type: %s).", key, key.getClass().getSimpleName(),
node.getClass().getSimpleName(), getClass().getSimpleName());
}

/* --------------- Shared --------------- */

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2359,7 +2359,10 @@ public void brokenEquality_eviction(BoundedLocalCache<Object, Int> cache,
assertThat(cache.estimatedSize()).isEqualTo(1);

var event = Iterables.getOnlyElement(TestLoggerFactory.getLoggingEvents());
assertThat(event.getMessage()).contains("An invalid state was detected");
assertThat(event.getFormattedMessage()).containsMatch(
"An invalid state was detected.*\\(key: \\d+, key type: MutableInt.*\\)");
assertThat(event.getThrowable().orElseThrow())
.isInstanceOf(IllegalStateException.class);
assertThat(event.getLevel()).isEqualTo(ERROR);

cache.data.clear();
Expand Down Expand Up @@ -2387,7 +2390,10 @@ public void brokenEquality_expiration(
assertThat(cache.estimatedSize()).isEqualTo(1);

var event = Iterables.getOnlyElement(TestLoggerFactory.getLoggingEvents());
assertThat(event.getMessage()).contains("An invalid state was detected");
assertThat(event.getFormattedMessage()).containsMatch(
"An invalid state was detected.*\\(key: \\d+, key type: MutableInt.*\\)");
assertThat(event.getThrowable().orElseThrow())
.isInstanceOf(IllegalStateException.class);
assertThat(event.getLevel()).isEqualTo(ERROR);

cache.data.clear();
Expand All @@ -2408,7 +2414,10 @@ public void brokenEquality_clear(BoundedLocalCache<Object, Int> cache, CacheCont
assertThat(cache.estimatedSize()).isEqualTo(1);

var event = Iterables.getOnlyElement(TestLoggerFactory.getLoggingEvents());
assertThat(event.getMessage()).contains("An invalid state was detected");
assertThat(event.getFormattedMessage()).containsMatch(
"An invalid state was detected.*\\(key: \\d+, key type: MutableInt.*\\)");
assertThat(event.getThrowable().orElseThrow())
.isInstanceOf(IllegalStateException.class);
assertThat(event.getLevel()).isEqualTo(ERROR);

cache.data.clear();
Expand Down Expand Up @@ -2490,7 +2499,9 @@ private static void testForBrokenEquality(BoundedLocalCache<MutableInt, Int> cac
key.decrement();

var e = assertThrows(IllegalStateException.class, () -> task.accept(key));
assertThat(e).hasMessageThat().contains("An invalid state was detected");
assertThat(e).hasMessageThat().containsMatch(
"An invalid state was detected.*\\(key: \\d+, key type: MutableInt.*\\)");

cache.data.clear();
}

Expand Down
4 changes: 2 additions & 2 deletions gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ ext {
versions = [
autoValue: '1.10.1',
cache2k: '2.6.1.Final',
checkerFramework: '3.32.0',
checkerFramework: '3.33.0',
coherence: '22.06.2',
commonsCompress: '1.23.0',
commonsLang3: '3.12.0',
Expand Down Expand Up @@ -64,7 +64,7 @@ ext {
univocityParsers: '2.9.1',
ycsb: '0.17.0',
xz: '1.9',
zstd: '1.5.4-2',
zstd: '1.5.5-1',
]
testVersions = [
awaitility: '4.2.0',
Expand Down
2 changes: 1 addition & 1 deletion gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
distributionUrl=https\://services.gradle.org/distributions/gradle-8.1-rc-2-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-8.1-rc-3-bin.zip
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
Expand Down

0 comments on commit 1eb57ac

Please sign in to comment.