AsyncMemoizer is caching exceptions

[akmalviya03]

Steps to reproduce

To reproduce the same issue you can refer the code sample given below.

Expected results

It should only cache successful result.

Actual results

It should not cache exception.

This callback is running
We have successfully cancelled future
Error First Exception: Future Cancelled
Error Second Exception: Future Cancelled

Code sample

Code sample

import 'package:async/async.dart';

late AsyncMemoizer<String> temporaryCache;
late CancelableOperation<String> cancelableOperation;

Future<void> main(List<String> arguments) async {
  temporaryCache = AsyncMemoizer<String>();
  cancelableOperation =
      CancelableOperation.fromFuture(_futureToRun(), onCancel: () {
    print('We have successfully cancelled future');
  });
  cancelableOperation.cancel();
   await getData().then((value) => print('First')).onError((error, stackTrace) => print('Error First $error'));

  cancelableOperation =
      CancelableOperation.fromFuture(_futureToRun(), onCancel: () {
        print('We have successfully cancelled future');
      });
  await getData().then((value) => print('Second')).onError((error, stackTrace) => print('Error Second $error'));
}

Future<String> _futureToRun() async {
  await Future.delayed(const Duration(seconds: 5));
  return Future.value("Temporary Cache");
}

Future<String> getData() async {
  return temporaryCache.runOnce(() async {
    print('This callback is running');
    String? value = await cancelableOperation.valueOrCancellation();
    if (value != null) {
      return value;
    }
    throw Exception('Future Cancelled');
  });
}

[lrhn]

Like for AsyncCache, not remembering an error is only meaningful if one expects the next call to behave differently. Which only the creator of the function can know.

So it would make sense to have a flag to control whether errors should be remembered.
But it's not an error to do so.

[lrhn]

Having looked at the code, I'm not sure changing the behavior of AsyncMemoizer is a good idea.
Or at least not doing it the most obvious way.

The way it's implemented today, a call to runOnce will immediately complete the Completer with the future returned by the function, long before that future completes.
If someone calles runOnce again, they immediately get the result future and isn't run a second time.

If the function then completes with an error later, only then is the completer fully completed.
Which means that "retrying on error" would still give the error to a number of callers, and if we want to clear the cache after that, we need to create a new Completer, because the old one is spent.

We can't just wait for the function's future to complete, to check if it's with an error, because then further calls to runOnce in the meantime will try to run function a second time.

So, at least for AsyncMemoizer, I think the best choice is to not change it, and maybe just document extra clearly that when you call runOnce, you get the result of that one run, whatever it is.

If you want the cupboard computation to retry, you have to build that into the function, not the cache.