LoadingCacheView.asMap()'s computeIfPresent blocks on Future completion while remove(key, value) doesn't

[jatcwang]

When attempting to implement an atomic removeIf operation for AsyncLoadingCache i found out that LoadingCacheView computeIfPresent seems to block, while remove doesn't. (computeIfPresent calls Async.getWhenSuccessful(oldValueFuture) while remove calls Async.getIfReady(oldValueFuture))

This is surprising to me because semantic-wise, I should be able to implement remove in terms of computeIfPresent (Neither of them should block)

Is there a reason for the blocking semantics for computeIfPresent?

Example (Excuse the pseudo Scala here):

// ... somewhere else
val asyncLoadingCache: AsyncLoadingCache

def removeIf(key: K, predicate: V => Boolean) = {

  val valueCacheView = asyncLoadingCache.synchronous().asMap()

  // I want to remove a cached value if it exists and matches a predicate
  // Since there's no `removeIf` method on ConcurrentMap interface I have to implement this method using either `computeIfPresent` or `remove`

  // Implementation using remove - doesn't block for future to complete but doesn't feel as clean
  val future = asyncLoadingCache.getIfPresent(key)
  if (future != null && future.isDone() && predicate(future.get())) {
    valueCacheView.remove(key, value)
  }
  else {
    // no value to check the predicate with at this instant - continue
  }

  // Or using getIfPresent - This blocks! :(
  valueCacheView.getIfPresent(key, (k: K, value: V) =>
    if (predicate(value)) {
      return null // returning null removes it from the 
    }
    else {
      return value // don't modify the value - this probably has the problem of refreshing the write time of this entry
    }
  )
}

[ben-manes]

The likely thought process was that if value in a remove(k, v) hasn't materialized yet then its not actually present. This would have been a valid optimization prior to the compute methods, but you're right it is now surprising. I should probably change all of those to be pessimistic by blocking.

[ben-manes]

This is surprising to me because semantic-wise, I should be able to implement remove in terms of computeIfPresent (Neither of them should block)

Oh, I misread this. A compute should block because it is memoizing a value so that concurrent calls don't do the same work in parallel. If a race caused it to compute but lose then the value has to be thrown away. That could be surprising, e.g. if the value is a resource that must be closed. The fact that remove(k, v) does not block on an in-flight compute(k, f) contradicts the behavior of ConcurrentHashMap. While valid by the interface, a user relying on that linearizable logic would equally surprised. So I think the correct fix is to make it block.

Potentially offering an Map<K, CompletableFuture<V>> might be handy if you wanted to have more non-blocking control. But I haven't seen a use-case yet, so its not offered.

[jatcwang]

I think the blocking semantics you mentioned here makes total sense given that's very common in the Java ecosystem. I think having the option to choose between a blocking interface or a non-blocking one will prove to be beneficial for everyone.

Is this simply just exposing LocalAsyncLoadingCache's cache variable? If o I'm happy to contribute a patch

[ben-manes]

It sounds like I need to review the remove and replace methods for linearizability, as I might have a similar prescreen in the synchronous case. The compute methods may prescreen for presence to lock on a load, but avoid that on a read. JDK9 will include prescreening for those methods, so the interleaving should follow expectations. I'll try to take a look at this over the weekend.

Unfortunately exposing a Map<K, CF<V>> is a little more work. A decorator must intercept all writes to add callbacks when the future completes. If it is successful the callback calculates the weight and expiration time of the entry. If not then it automatically removes it. We then need to add a test suite for proper coverage. Ideally we could refactor AsMapTest to somehow work with sync/async values or worst case fork it.

[ben-manes]

I strengthened the methods to block rather than optimistically skip. I'll try to look into the async asMap view soon.

[ben-manes]

I've been working on this lately, sorry its taken so long. This will introduce an asynchronous map view on the newly introduced AsyncCache interface, which AsyncLoadingCache extends. For backwards compatibility, it will have to be a default method throwing an unsupported exception. But it will be implemented by Caffeine, so that shouldn't effect anyone.

You'll now have ConcurrentMap<K, CompletableFuture<V>> asMap(), which is the type of the underlying map. The only caveat is that CompletableFuture does not implement an equals and hashCode. This means that any methods that compare by value will be identity based, e.g. containsValue(value) and replace(key, oldValue). I think that's fair, and you'll still have the synchronous map view for convenience.

So far it passes Guava's TestLib, which has robust collections testing. I am still porting a copy of AsMapTest, which is quite large. Once that's functional, I'll have to add a few more test cases for the exceptional cases where the newly added future is automatically removed. Mostly just grunt work at this point to ensure proper test coverage.

[ben-manes]

Released 2.7