Fix/async memoizer storing exception

[akmalviya03]

Earlier asyncMemoizer was caching exceptions.
With new changes in runOnce method of AsyncMemoizer class will no longer store exception.
By default AsyncMemoizer will store exception, by setting _canCacheException to false. It won't store exception.

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

[natebosch]

I'm worried about what this does to the future member of this class - it disconnects it from the operation.

I'm iffy overall on this behavior. If we do decide to move forward I'd rethink the naming some.

Can you describe your use case?

[akmalviya03]

We are giving user a way to choose whether he want to persist the error or not. Right now we don't have a way to do this.

Let's take an example, suppose you are building an application which makes an API call to fetch top 50 news. And you want to memoize the data received from the API call. Unfortunately you got an exception might be a socket exception. You want to resend the API call but you are getting same error because AsyncMemoizer is persisting exception. I hope this clears the purpose of this PR.

[lrhn]

Thinking about this more, it feels a little inconsistent to provide a failure, but not cache it.
That makes the AsyncMemoizer into a retry functionality.
I'd rather have a retry functionality by itself.

Alterantively we can provide a clear() function on the memoizer. Then someone getting an error can choose to clear the memoizer, rather than putting that logic into the memoizer.
(That can have race conditions, though. If multiple listeners get an error, and they all clear and retry, then they may reset another already started operation. So possibly the clear() should only work while a computation is in progress. Which means the memoizer needs to know that, but I guess it already does, since it knows whether there was an error or not.)

A retry functionality could be something like:

import "dart:async";
/// Runs [computation] until it succeeds.
///
/// Runs [computation] and returns a future which completes with the
/// non-error result of that computation.
/// If the computation throws, it is retried, at most up to [max] times if [max] is provided,
/// and only on a non-error result, or reaching [max] failed tries, is the returned future
/// completed.
/// If [delay] is provided, it's called to provide a delay to insert after a failed attempt,
/// with the argument being the number of failed attempts before it, starting at 1.
/// This allows a user-configurable backoff strategy.
Future<T> retry<T>(Future<T> computation(),
    {int? max, Duration Function(int)? delay}) {
  var attempts = 0;
  var result = Completer<T>.sync();
  void retry() {
    void fail(Object error, StackTrace stack) {
      attempts += 1;
      if (attempts == max) {
        result.completeError(error, stack);
        return;
      }
      if (delay == null) {
        retry();
      } else {
        var pause = delay(attempts);
        Timer(pause, retry);
      }
    }

    try {
      computation().then((v) {
        result.complete(v);
      }, onError: fail);
    } catch (e, s) {
      fail(e, s);
    }
  }

  retry();
  return result.future;
}

Then avoid caching failures by not having failures:

  var once = AsyncMemoizer(retry(possbilyFailingOperation));

It does mean that all queries will wait for a real result, and won't know about failures.

[natebosch]

I don't think you can build these semantics with retry() if you specifically do want to receive the incremental error (maybe to show a "retrying" instead of a "loading" message) and have control over how long you wait until the next try.

I also don't know how much clear() helps - though I do have a preference for building this as new behavior over building this as a tweak to existing behavior.

Your thoughts about race conditions brings up other issues. The doc comment for runOnce needs an overhaul if we accept this change. The behavior is different depending on this setting.

diff --git a/lib/src/async_memoizer.dart b/lib/src/async_memoizer.dart
index c05c927..4cf166f 100644
--- a/lib/src/async_memoizer.dart
+++ b/lib/src/async_memoizer.dart
@@ -36,9 +36,20 @@ class AsyncMemoizer<T> {
   /// Whether [runOnce] has been called yet.
   bool get hasRun => _completer.isCompleted;
 
-  /// Runs the function, [computation], if it hasn't been run before.
+  /// Runs the function, [computation], if it hasn't been run before, or if it's
+  /// currently running and `canCacheException` is `false`.
   ///
-  /// If [runOnce] has already been called, this returns the original result.
+  /// If `canCacheException` is `true` and  [runOnce] has already been called,
+  /// this returns the original result.
+  ///
+  /// If `canCacheException` is `false` and [runOnce] has already been called,
+  /// the behavior depends on whether the prior call has completed, and whether
+  /// it resulted in an exception.
+  /// If the previous computation completed successfully, this returns the
+  /// original result.
+  /// If the previous computation is ongoing, or the previous computation
+  /// resulted in an error, this starts a new run of the callback and returns
+  /// the result of the new call.
   Future<T> runOnce(FutureOr<T> Function() computation) {
     if (!hasRun) _completer.complete(Future.sync(computation));
     return future;

I don't know if that's the behavior users would expect though - I think most users would expect a previous ongoing computation to be awaited instead of starting a new one.

Overall I think I'm leaning towards not making this change as is. I don't currently have a proposal for an alternative though.

[natebosch]

I don't think the async cache class in the other PR has the same race condition concerns I have here since it already had the ephemeral implementation with the same behavior - so I think we should be good to land #258 regardless of the decision here.

[akmalviya03]

We can create a separate class with similar functionality like asyncMemoizer and cache invalidation. Or we can rename runOnce to some other name which is more meaningful. If we are going to keep cache invalidation functionality.

[mosuem]

Closing as the dart-lang/async repository is merged into the dart-lang/core monorepo. Please re-open this PR there!