Skip to content
This repository has been archived by the owner on Oct 17, 2024. It is now read-only.

Make ephemeral async cache clear sooner. #207

Merged
merged 1 commit into from
Mar 21, 2022
Merged

Make ephemeral async cache clear sooner. #207

merged 1 commit into from
Mar 21, 2022

Conversation

lrhn
Copy link
Contributor

@lrhn lrhn commented Mar 14, 2022

The AsyncCache.ephemeral() cache would invalidate
the fetch cache after a Duration.zero wait.
That allows a large number of microtasks to happen between
the fetch completing and the cache being invalidated.

Instead the cache is now invalidated immediately when the fetched
request completes.

Fixes dart-lang/core#335

The `AsyncCache.ephemeral()` cache would invalidate
the `fetch` cache after a `Duration.zero` wait.
That allows a large number of microtasks to happen between
the `fetch` completing and the cache being invalidated.

Instead the cache is now invalidated immediately when the fetched
request completes.
@lrhn lrhn requested a review from natebosch March 14, 2022 15:39
Comment on lines +66 to +67
return _cachedValueFuture ??= callback()
..whenComplete(_startStaleTimer).ignore();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this behavior change intentional? Do we still want to call _startStaleTimer if _cachedValueFuture was non-null?

Suggested change
return _cachedValueFuture ??= callback()
..whenComplete(_startStaleTimer).ignore();
return (_cachedValueFuture ??= callback())
..whenComplete(_startStaleTimer).ignore();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is actually correct, and the original wasn't. The original could trigger multiple invalidations (at the same time, _duration after the future completed, so probably not a problem). One is sufficient.

@natebosch
Copy link
Contributor

This LGTM, but let's hold off on merging until we figure out why an earlier change broke some tests.

#204 (comment)

@lrhn
Copy link
Contributor Author

lrhn commented Mar 21, 2022

I believe the earlier change was fixed, so landing.

@lrhn lrhn merged commit 9309ebe into master Mar 21, 2022
mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 14, 2024
The `AsyncCache.ephemeral()` cache would invalidate
the `fetch` cache after a `Duration.zero` wait.
That allows a large number of microtasks to happen between
the `fetch` completing and the cache being invalidated.

Instead the cache is now invalidated immediately when the fetched
request completes.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Ephemeral AsyncCache is not truly ephemeral
2 participants