Skip to content
This repository was archived by the owner on Mar 14, 2025. It is now read-only.

Use Caffeine for request aggregation #6

Merged
merged 7 commits into from
Jan 21, 2022

Conversation

SailReal
Copy link
Member

As we found out and demonstrated using GuavaCacheTest.java that the Guava-Cache isn't completely thread-safe, we're switching to Caffeine for metadata request deduplication:

The new MetadataRequestDeduplicationDecorator decorates an existing CloudProvider by de-duplicating identical itemMetadata and list-requests so that the delegate is called only once until the future is completed.
Furthermore, quota-requests are cached for a duration of default 10 seconds (can be set using org.cryptomator.cloudaccess.metadatacachingprovider.timeoutSecond).

In projects that use this library, we will write a factory that selects the best metadata decorator, currently MetadataCachingProviderDecorator or MetadataRequestDeduplicationDecorator.

Guava's cache is not thread-safe, see GuavaCacheTest to verify.
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work so far. Just some small suggestions

@SailReal SailReal merged commit 4a31f01 into develop Jan 21, 2022
@SailReal SailReal deleted the feature/use-caffeine-for-request-aggregation branch January 21, 2022 15:59
@ben-manes
Copy link

As we found out and demonstrated using GuavaCacheTest.java that the Guava-Cache isn't completely thread-safe, we're switching to Caffeine for metadata request deduplication:

I think you mean that invalidate(key) does not discard an in-flight load (google/guava#1881)? That is thread-safe but not linearizable, which can cause surprises if one depends on that to avoid stale loads. Caffeine does support that (and verified by Lincheck) for keyed operations.

Unfortunately we cannot natively do that for invalidateAll() / Map.clear() due to in-flight loads being suppressed by ConcurrentHashMap's iterators (which guarantee visibility of inserted mappings). As we need to maintain our own metadata, linearizable bulk operations would require forking the hash table for internal access (see CHM's clear() implementation). That caveat is simple to workaround for AsyncCache since the load is performed by the future, allowing the mapping to be established immediately and one can easily rework the logic to maintain linearizable clear() semantics. If that is of interest / need for your usage, we can discuss that further. In short, you would use either JCIP-style memoization instead of computeIfAbsent style, or have the load wait on a flag to force the computation to starts after mapping was established, or include a generational id as part of the key.

SailReal added a commit that referenced this pull request Jan 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants