Skip to content

fix: async cache storing exception fixed #548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkgs/async/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

- Fix `StreamGroup.broadcast().close()` to properly complete when all streams in the group close without being explicitly removed.
- Run `dart format` with the new style.

- Can decide `fetch` method of `AsyncCache` will store exception or not
by using `cacheErrors` property.
-
## 2.13.0

- Fix type check and cast in SubscriptionStream's cancelOnError wrapper
Expand Down
31 changes: 27 additions & 4 deletions pkgs/async/lib/src/async_cache.dart
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ class AsyncCache<T> {
/// Cached results of a previous [fetch] call.
Future<T>? _cachedValueFuture;

/// Whether the cache will keep a future completed with an error.
///
/// If `false`, a non-ephemeral cache will clear the cached future
/// immediately if the future completes with an error, as if the
/// caching was ephemeral.
/// _(Ephemeral caches always clear when the future completes,
/// so this flag has no effect on those.)_
final bool _cacheErrors;

/// Fires when the cache should be considered stale.
Timer? _stale;

Expand All @@ -44,14 +53,18 @@ class AsyncCache<T> {
/// The [duration] starts counting after the Future returned by [fetch]
/// completes, or after the Stream returned by `fetchStream` emits a done
/// event.
AsyncCache(Duration duration) : _duration = duration;
/// If [cacheErrors] is `false` the cache will be invalidated if the [Future]
/// returned by the callback completes as an error.
AsyncCache(Duration duration, {bool cacheErrors = true})
Copy link
Member

Choose a reason for hiding this comment

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

Any naming we can use that would make the flag default to false?
Just because it's usually a good idea to have false as the default value.

Maybe bool ephemeralErrors = false. (Ouch, would hate to have to write that).
flushErrors?

Copy link
Author

Choose a reason for hiding this comment

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

What about discardErrors?

: _duration = duration,
_cacheErrors = cacheErrors;

/// Creates a cache that invalidates after an in-flight request is complete.
///
/// An ephemeral cache guarantees that a callback function will only be
/// executed at most once concurrently. This is useful for requests for which
/// data is updated frequently but stale data is acceptable.
AsyncCache.ephemeral() : _duration = null;
AsyncCache.ephemeral(): _duration = null, _cacheErrors = true;
Copy link
Member

Choose a reason for hiding this comment

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

The _cacheErrors here feels like it should be false (since it doesn't cache errors, because it doesn't cache anything). It's invisible to the user, so it doesn't really matter, and I guess it just means "don't special-case errors".
It's OK, just a little confusing.

I guess there are really three modes:

  • Ephemeral everything.
  • Ephemeral errors, persistent values.
  • Persistent everything.

covered by two constructors. Should we just have three constructors?


/// Returns a cached value from a previous call to [fetch], or runs [callback]
/// to compute a new one.
Expand All @@ -62,8 +75,18 @@ class AsyncCache<T> {
if (_cachedStreamSplitter != null) {
throw StateError('Previously used to cache via `fetchStream`');
}
return _cachedValueFuture ??= callback()
..whenComplete(_startStaleTimer).ignore();
if (_cacheErrors) {
return _cachedValueFuture ??= callback()
..whenComplete(_startStaleTimer).ignore();
} else {
return _cachedValueFuture ??= callback().then((value) {
_startStaleTimer();
return value;
}, onError: (Object error, StackTrace stack) {
invalidate();
throw error;
});
}
}

/// Returns a cached stream from a previous call to [fetchStream], or runs
Expand Down
20 changes: 20 additions & 0 deletions pkgs/async/test/async_cache_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,26 @@ void main() {
cache = AsyncCache(const Duration(hours: 1));
});

test('should not fetch when callback throws exception', () async {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure "fetch" is the verb I'd use here (aka: I don't know what the description means).
It's consistent with the other tests. I also can't read those.

That's not really your issue to fix, though. (Should we say "recompute" instead of "fetch". Why is the method called fetch to begin with, seems to assume a specific use-case.)

Copy link
Author

@akmalviya03 akmalviya03 Apr 25, 2025

Choose a reason for hiding this comment

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

Initially I also thought the same, to rename it to something more generic.

cache = AsyncCache(const Duration(hours: 1), cacheErrors: false);
Future<String> asyncFunctionThatThrows() {
throw Exception();
}

var errorThrowingFuture = cache.fetch(asyncFunctionThatThrows);
await expectLater(errorThrowingFuture, throwsA(isException));

FakeAsync().run((fakeAsync) async {
var timesCalled = 0;
Future<String> call() async => 'Called ${++timesCalled}';

expect(await cache.fetch(call), 'Called 1');

fakeAsync.elapse(const Duration(hours: 1));
expect(await cache.fetch(call), 'Called 2');
});
});

test('should fetch via a callback when no cache exists', () async {
expect(await cache.fetch(() async => 'Expensive'), 'Expensive');
});
Expand Down