Seemingly inconsistent invocations of eviction and removal listeners.

[valmaltsev]

It seems that for some operations, like refresh after write and explicit invalidation, only removal listener is invoked, but for other operations, like expire after access and size based eviction, both eviction and removal listeners are invoked. Does that mean that when we have only removal listener invoked there is no way to do something with the old value atomically with, for example, reload action? I would expect to have either both listeners invoked every time or just one of them, specific to an action, but this different behavior of different actions is a bit puzzling. Could you please give a bit more detailes why it was implemented this way?
I'm using the following configuration for testing
Caffeine.newBuilder().maximumSize(SIZE).expireAfterAccess(Duration.ofSeconds(EXPIRE)).refreshAfterWrite(Duration.ofSeconds(REFRESH)).evictionListener((String key, Object value, RemovalCause cause) -> { evictionListener(key, value, cause); }).removalListener((String key, Object value, RemovalCause cause) -> { removalListener(key, value, cause); }).executor(Runnable::run).build();

[ben-manes]

For explicit invalidation you can use the asMap().compute methods, where a null return value causes the entry to be removed. As the computation is atomic, this can perform custom logic and it is more idiomatic as how you would do so in with ConcurrentHashMap (versus a plain remove(key)).

For refresh this operation is computed asynchronously. It invokes CacheLoader#reload(key, oldValue) via its asyncReload default method. There is no atomic listener here, but the intent is that you already had the old value to evaluate with. If the entry changes so that the reload is invalid then the replacement is discarded (with the new value notified as replaced). There isn't an atomic listener for this swap when successful, but it's probably a bad usage if one was needed for that use-case.

The background is that an issue in Guava was the lack of an atomic listener when an entry was automatically removed due to eviction. That might be needed for stateful resources like a local file, where it could be loaded anew before the async removal listener is run. In some other caches they have a concept of a CacheWriter to intercept put / remove operations, so we first tried a similar interface. That was quirky with the Map interface and its newer compute methods, and wasn't lambda friendly. When searching open-sourced code the primary usages when searching open-sourced code was for intercepting evictions, with a few cases where Map computations was a much better fit for the explicit writes. Therefore the cache writer was deprecated and removed for this more narrow use-case, and Map computations were evangelized as more idiomatic in modern Java code.

For complex cases use cache.asMap().compute and friend, as fully supported, powerful, and understandable to other developers. If you need to respond atomically to evictions then also use an eviction listener. And for a reload, if it can leverage the old value then override the method in CacheLoader to leverage that in your calculation. I hope that clears things up.

[valmaltsev]

Thank you for the explanation, it helps a lot. The specific issue I had was with a reference counter on the cached object. Sometimes it was not decremented at all, when I used only eviction listener, and sometimes it was decremented twice, when I used both listeners. After realizing that eviction listener is not invoked for every removal operation I just redesigned the code to not rely on atomicity and to use only removal listener.

[ben-manes]

I remember now why eviction listener is not merely an atomic / synchronous removal listener for all removal types. The problem is that would be incompatible with AsyncCache because the entry maps to a possibly in-flight CompletableFuture. Therefore an explicit removal would need to block the caller waiting for the future to complete (not good) or be an incompatible feature (as CacheWriter was). An in-flight entry is not eligible for eviction, so notifying the eviction listener won't need to wait and is therefore compatible with both cache types. That means if we were to add a synchronous removal listener it would be limited to synchronous caches, but so far we haven't found a strong reason for including when reviewing how CacheWriter was used and lack of negative feedback when it was deprecated.