Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed behavior of returned empty maps from LocalAsyncCache #864

Closed
mranders-hltv opened this issue Feb 1, 2023 · 4 comments
Closed

Changed behavior of returned empty maps from LocalAsyncCache #864

mranders-hltv opened this issue Feb 1, 2023 · 4 comments

Comments

@mranders-hltv
Copy link

The change of LocalAsyncCache#composeResult, which swaps Collections.emptyMap() for Map.of() has resulted in changed behavior of the returned maps.

Collections.emptyMap() does allow looking up null keys (resulting in null values), while Map.of() returns Javas MapN, which throws an exception when null is looked up (source below):

public V get(Object o) {
  if (size == 0) {
    Objects.requireNonNull(o);
    return null;
  }
  <SNIP>

This means that the following code fails with an exception when using Caffeine 3.1.2, instead of getting null from the map.

LoadingCache<Object, Object> loadingCache = Caffeine.newBuilder().buildAsync(key -> key).synchronous();
Map<Object, Object> loaded = loadingCache.getAll(Collections.emptyList());
loaded.get(null); //Exception

Even though Map#get does not guarantee that null keys can be used, Caffeine does not allow looking up null keys, and LoadingCache#getAll explicitly states that the returned map "will never contain null keys or values", it is still a small but subtle change in behavior, which can cause runtime problems for users. Especially because it only occurs in the combination of a cache lookup of an empty Iterable, and a lookup of a null key in the returned Map.

@ben-manes
Copy link
Owner

oh, good catch. I knew of that annoying difference, but forgot at the time. I'll restore this and add unit tests to assert the null lookup behavior.

ben-manes added a commit that referenced this issue Feb 2, 2023
…864)

While a null lookup is not allowed for cache the operations (e.g. asMap.get), if
an operation returned a map then this might not be consistant. Originally all of
these cases used Collections' emptyMap() or unmodifiableMap(m), but a few cases
switched to Map's of() or copyOf() alternatives. The former allows for null
queries whereas the latter does not, and this could vary depending on if the
result was populated. For consistency and retaining the original behavior, this
is restored to the Collections' methods.
ben-manes added a commit that referenced this issue Feb 2, 2023
…864)

While a null lookup is not allowed for cache the operations (e.g. asMap.get), if
an operation returned a map then this might not be consistant. Originally all of
these cases used Collections' emptyMap() or unmodifiableMap(m), but a few cases
switched to Map's of() or copyOf() alternatives. The former allows for null
queries whereas the latter does not, and this could vary depending on if the
result was populated. For consistency and retaining the original behavior, this
is restored to the Collections' methods.
@ben-manes
Copy link
Owner

ben-manes commented Feb 4, 2023

I looked into adding unit tests for this case, but I believe it is already covered by LoadingCacheTest.refresh_expired. In that variant the entry expires before the reload completes, which results in it being evicted and the refresh discarded. That's an explicit refresh, but an automatic one behaves equivalently. I may look into adding more explicit tests just to avoid any regressions by clarifying the scenarios. So I believe you simply need to upgrade to resolve the issue.

@ben-manes
Copy link
Owner

Sorry, commented on the wrong issue!

@ben-manes
Copy link
Owner

Released in 3.1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants