FeatureRequest: enable a Future<Void> for refresh()

[boschb]

Hi Ben,

Currently there is no way to know when a value is refreshed after calling refresh() on a LoadingCache. Some synchronization techniques could be utilized with a CacheLoader, but I still don't think that guarantees exact entry depending on when context switching occurs.

Thanks!

[ben-manes]

Can you provide a little more context on how you would use this?

Unfortunately due to semver, this API change would require a major release. I plan on 3.0 being these types of tweaks and targeting jdk9 (no unsafe) whenever it ships.

[boschb]

Sure thing.

My scenario is I have a Multimap<Key,Value> that I use as truth. I want to provide an immutable 'snapshot' of the map using a Cache<Key,Set<Value>> with a CacheLoader that simply synchronizes on the Multimap and builds an ImmutableSet from the current values (for a given key). This might seem like overkill, but the idea is that I will always have a 'snapshot' set that I can iterate over (at high frequency) without blocking the underlying Cache or Multimap. Adds and removes are less frequent and not under the same blocking requirements. Also my service allows for non-parity between the cache and map (for short a short period of time) as best effort, mostly accurate, snapshots provide all we need.

By utilizing the features of refresh() where a new value is loaded in the background while the old value continues to serve, this gives me exactly the behavior I need.

The only catch that I have is that I need to be able to gate (especially during tests) the point at which a new entry added to the Multimap actually is present in the underlying Cache.

[boschb]

Regarding versioning, wouldn't a @IgnoreReturnValue and ListenableFuture be safe? method is already void. I would even be ok with a completely new method. It's also entirely possible I don't know what i'm talking about though in regards to semver as I'm not that familiar.

Also FWIW, if this has to be a AsyncCache thing that's fine too (The line is a little grey regarding refresh())

[ben-manes]

To clarify, is this needed for testing or the implementation? It sounds like testing only.

The API change would mean any other implanting class would not compile. I'd also like to ensure changes carry their weight, as an API is long lived.

[boschb]

Technically both. Though the implementation in practice I think avoids this race probably every time because of network latency. The timing hole still exists though (and tests prove it) so it does make me feel more correct to use it if I can.

I was hoping that we could just bubble up the future from the CacheLoader easy peasy:
https://github.com/ben-manes/caffeine/blob/master/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalLoadingCache.java#L158

I see your point about breaking other implementors though. Api wise though I think it makes sense to bubble up this future from the loader, or maybe even move the insert logic to the loader itself by passing the Cache to the reload function... It's a tough call. They are pretty interlinked, but this def is a Loading/Loader specific feature... Your thoughts?

[ben-manes]

I think you could use Awaitility so that your tests tolerate this race.

From your description, it also sounds like alternatives to refreshing may work. If you use ConcurrentMap<K, ImmutableSet<V>> instead, then the cache might be unnecessary. Or perhaps some of the asMap view write methods useful if you insert into the cache after updating the Multimap.

Right now I'm neutral on this change, but it would have to wait until 3.0 so hopefully we can find a workaround for now.

[boschb]

Awaitility wont work as this is happening in integration/loadtesting too... however yes I can probably fall back on a ConcurrentMap and just do the updates inline manually. I may just do that actually. I've gotten so used to just using a Cache instead of a Map because of the nice features.

I would say I think there is still merit to bubbling up the Future (especially since it's there). Can't hurt, and I would imagine any implementor has similar bits of code with the Loader that you do. there's always immediateFuture too. I'm not sold on putting Future stuff in the non-async version though, even though the documentation says it can happen asynchronously.

Anyways thanks for the help and let me know if it makes it in 3.0.
Cheers,
Bobby

[costimuraru]

👍
I was a bit surprised to see that the refresh() method returns void and there is no way to be notified when the value is loaded. In my use case if I detect my value is stale, I want to refresh() and block until it's loaded.

[ben-manes]

It never came up for Guava's Cache, perhaps because chained futures was a less commonly used idiom. Because of that, it didn't occur to me that users would take advantage of it. Most often refreshAfterWrite was relied on, instead.

I added this to the roadmap. Due to the API change it will require the 3.0 release, which will JDK9 based (to leverage VarHandles). If I can toggle between Unsafe and VarHandles, I might make it JDK8 compatible.

[costimuraru]

I believe I got it to work using invalidate().

CacheValue cachedValue = lookupCache.get(cacheKey);
if (cachedValue.isExpired()) {
    // Refresh value.
    lookupCache.invalidate(cacheKey);
    cachedValue = lookupCache.get(cacheKey);
}

The only thing that bugs me are the cache statistics. In the case when the value is expired (related to #114), the above code will generate 2 cache requests (1 hit and 1 miss). Ideally, this should be 1 cache request with 1 miss.

By the way, great job with this library! This is gold.

[ben-manes]

To avoid race conditions of an load/invalidation storm, you should use cache.asMap().remove(cacheKey, cachedValue).

I think variable expiration will solve your problem better, so hopefully I'll get it stabilized in a week or two.

[costimuraru]

Thanks, Ben! I've updated the code with your recommendation.
The variable expiration would be cool and the hash timer wheel sounds intriguing.

[ben-manes]

2.5.0 added support for variable expiration. This would resolve @costimuraru's use-case.

I'm leaving this issue open for @boschb's scenario, which is to return the internal refersh future to the client so that they can add their own chain. That requires changing the API, so we have to wait for v3.0. The hold off on that is to make it a JDK9 release, though with the Jigsaw debacle I don't know what the timeline is.

[ben-manes]

Java 9 was released today, so my goal will be to release v3 by the end of the year. Hopefully sooner.

There are a bunch of tasks in the roadmap. Most are simple, except for batch refresh and JCache expiration overhaul. Most likely I'll keep punting on the refresh task, since that can be added scaffolded onto the interfaces using default methods.

[blizzard384]

This feature would be much appreciated. I have an usecase when I want to send push notification after cache refresh() is finished.

[ben-manes]

Sorry for not making progress on v3. I think all the blockers are resolved, so I am reviving the branch and working through these tasks.

I plan on returning CompletableFuture<V>, where this future is the instance supplied by the cache loader (via asyncReload or asyncLoad, as appropriate). There are some minor implications of this,

1. This matches AsyncCache calls which always hand back the user supplied future. This is more flexible if the future needs to be operated on, e.g. cancelation, or if managed elsewhere where the reference equality matters. A chained future doesn't propagate upstream, so I try to not return those for less surprising usage.
2. This means that adding chained logic goes against the original future. Because CompletableFuture chains as a stack, the last chained action is performed first (LIFO). That has a surprise where Caffeine's callbacks to update the hashmap entry is performed after the user's callbacks.
3. The reloaded value may be discarded by the cache if the entry was updated or removed, so the refresh might not be applied anyways. This is to avoid the ABA problem and matches Guava's behavior.
4. The feature request suggests returning Void, but I see no harm in the actual V value if the caveats are understood.

[boschb]

Hi Ben,

Happy New Year!

I think this mostly sounds all fine.

My only comment is for #2 regarding LIFO. In practice the timing of issuing the callback before the refresh completes seems problematic. This would be a timing hole (though tiny) in testing and in production it appears, but maybe i'm reading it wrong. Having a V instead of Void sorta avoids the need to access the entry via GET, but I could easily see a caller waiting for refresh() only to then access the current value in the map. If this is still racing, it could yield null or another value that got applied after the refresh(). Short of contention situation, i would expect refresh(K) == get(K) without race conditions.

I see this gets quite complex with contention, but maybe its possible to return future that delegates to the completion of future modifying the map, but uses the value from the refresh() operation? This also seems a little round about to me, but maybe it's more correct? Hard to say...

Thoughts?

[ben-manes]

Released in 3.0