FutureSignal awaiting an AsyncSignal.future is executed too often

[Yegair]

Description

I have a FutureSignal that is computed by awaiting the value of another FutureSignal.
The computed signal is executed twice, although the source signal only ever emits a single value.

Reproduction

test('computed future emits once', () async {
  int calls = 0;
  int signalCalls = 0;
  int computedCalls = 0;
  int heavyComputationCalls = 0;

  Future<int> future() async {
    calls++;
    await Future.delayed(const Duration(milliseconds: 5));
    return 10;
  }

  final signal = futureSignal(() async {
    signalCalls++;
    return await future();
  });

  Future<int> heavyComputation(int value) async {
    heavyComputationCalls++;
    await Future.delayed(const Duration(milliseconds: 5));
    return value * 2;
  }

  final computed = futureSignal(
    () async {
      computedCalls++;
      final value = await signal.future;
      return await heavyComputation(value);
    },
    dependencies: [signal],
  );

  final result = await computed.future;

  expect(result, 20, reason: 'unexpected result');
  expect(calls, 1, reason: 'the future function was called an unexpected number of times');
  expect(signalCalls, 1, reason: 'the signal function was called an unexpected number of times');
  expect(heavyComputationCalls, 1, reason: 'the heavy computation function was called an unexpected number of times');
  expect(computedCalls, 1, reason: 'the computed function was called an unexpected number of times');
});

This test will fail with

Expected: <1>
  Actual: <2>
the heavy computation function was called an unexpected number of times

---

I assume the reason is that future signals have a value of AsyncState that does change twice (AsyncLoading -> AsyncData).

However, I am wondering if there might be a way to improve the AsyncSignal.future in a way that causes the dependent signals to be computed only once the AsyncData (or AsyncError) is emitted. This would be especially helpful in cases where the computed future signal does some heavy lifting (like running something on a separate isolate).

[rodydavis]

Can you also
Try with computedFrom/computedAsync?

[Yegair]

Tried both, for example here is the version with the computedFrom

test('computed future emits once', () async {
  int calls = 0;
  int signalCalls = 0;
  int computedCalls = 0;
  int heavyComputationCalls = 0;

  Future<int> future() async {
    calls++;
    await Future.delayed(const Duration(milliseconds: 5));
    return 10;
  }

  final signal = computedFrom([], (_) async {
    signalCalls++;
    return await future();
  });

  Future<int> heavyComputation(int value) async {
    heavyComputationCalls++;
    await Future.delayed(const Duration(milliseconds: 5));
    return value * 2;
  }

  final computed = computedFrom([signal], (_) async {
    computedCalls++;
    final value = await signal.future;
    return await heavyComputation(value);
  });

  final result = await computed.future;

  expect(result, 20, reason: 'unexpected result');
  expect(calls, 1, reason: 'the future function was called an unexpected number of times');
  expect(signalCalls, 1, reason: 'the signal function was called an unexpected number of times');
  expect(heavyComputationCalls, 1, reason: 'the heavy computation function was called an unexpected number of times');
  expect(computedCalls, 1, reason: 'the computed function was called an unexpected number of times');
});

The behaviour is the same in all cases. TBH I did expect that, since computedFrom and computedAsync do nothing special besides having a slightly different API, which in and of itself is a very good thing if you ask me.

[rodydavis]

Thanks for verifying that!

I think there is a race because the first signal starting immediately and then call again later before the first one completes.

Would love some input on how you would like the API to improve or even some suggested fixes!

Ideally we do as minimal calls to the future as necessary.

[Yegair]

Would love some input on how you would like the API to improve ...

I do like the API as it is. I wouldn't change it at all, if possible.

I.e. this is pretty much exactly how I'd want to write my async computed code:

computedFrom([someSignal, otherSignal], (_) {
  final someValue = await someSignal.future;
  final someDerivedValue = await doSomething(someValue);
  final otherValue = await otherSignal.future;
  return await doSomethingElse(otherValue, derivedValue);
});

... or even some suggested fixes!

Disclaimer: I have not yet fully understood how Signals and especially computed works under the hood, so maybe I will now suggest something that is completely wrong and/or misses the whole point. Nevertheless, here is what I currently have in my head:

• I assume every call to a Signals get value is tracked, so the Signal knows whom to notify if it's value is changed. In this case this is done implicitly through calling signal.future which simply calls signal.value internally.
• I also assume that when a Future signal is computed there will be two calls to set value, roughly equivalent to:

  this.value = AsyncState.loading(...); // --> notifies dependents
  final newValue = await fn();
  this.value = AsyncState.data(newValue); // --> notifies dependents

• Hence the dependent computedFrom will be notified twice, which is necessary, since the notifying signal doesn't know whether the dependent wants to see the AsyncLoading and later the AsyncData or just the AsyncData once it is available.
• Thus I come to the conclusion, that signal.future must use a different signal under the hood that "skips" the this.value = AsyncState.loading(...); step, in order to not notify its dependents until the internal async function has returned.

I did look through the implementation of StreamSignal and AsyncSignal and tried out a few things, but so far I have nothing that would be worth a PR.

E.g. What I had in mind was something like this as part of packages/signals_core/lib/src/async/signal.dart

class AsyncSignal<T> extends ... {
  AsyncSignal(...) {
    _internalCompleter = Completer<T>();
    _internalFuture = signal(_internalCompleter.future);
  }

  // ...

  late Completer<T> _internalCompleter;
  late Signal<Future<T>> _internalFuture;

  // ...

  Future<T> get future {
    return _internalFuture.value;
  }

  // ...

  void setValue(T value) {
    batch(() {
      this.value = AsyncState.data(value);
      if (_internalCompleter.isCompleted) {
        _internalCompleter = Completer<T>();
        _internalFuture.value = _internalCompleter.future; // --> notifies async dependents
      }
      _internalCompleter.complete(value);
    });
  }

  // ...
}

[Yegair]

FWIW, I have created a draft PR with the proposed change. It does solve the test that I have written, but it does break some other tests, so it can't be the final solution.

[Yegair]

Also note that I had to introduce a new API for specifying computedFrom dependencies:

 computedFrom(
-  [someAsyncSignal],
+  [someAsyncSignal.completion],
   (_) async { 
-    return await someAsyncSignal.future;
+    return await someAsyncSignal.completionFuture;
   },
 );

I think it should be possible to merge the existing .future with the new .completionFuture, but something like the .completion for dependencies might still be necessary.

However, before putting more time into the solution, I do need your opinion on it, since it might be going completely into the wrong direction.

[jinyus]

I tried this with my package and the test passes, it looks like the .future method still references the value so it will not ignore the loading state. @Yegair is correct, you need an internal signal that only tracks the data and error states. You can copy my implementation or I could port it if you approve.

https://github.com/jinyus/dart_beacon/blob/f4d7d1ff01ff898b7425083d55678ff0e80739e0/packages/state_beacon_core/lib/src/beacons/future.dart#L133

test('derived future emits once', () async {
    int calls = 0;
    int signalCalls = 0;
    int computedCalls = 0;
    int heavyComputationCalls = 0;

    Future<int> future() async {
      calls++;
      await Future.delayed(const Duration(milliseconds: 5));
      return 10;
    }

    final signal = Beacon.future(() async {
      signalCalls++;
      return await future();
    });

    Future<int> heavyComputation(int value) async {
      heavyComputationCalls++;
      await Future.delayed(const Duration(milliseconds: 5));
      return value * 2;
    }

    final computed = Beacon.future(
      () async {
        computedCalls++;
        final value = await signal.toFuture();
        return await heavyComputation(value);
      },
    );

    final result = await computed.toFuture();

    expect(result, 20, reason: 'unexpected result');
    expect(calls, 1,
        reason: 'the future function was called an unexpected number of times');
    expect(signalCalls, 1,
        reason: 'the signal function was called an unexpected number of times');
    expect(heavyComputationCalls, 1,
        reason:
            'the heavy computation function was called an unexpected number of times');
    expect(computedCalls, 1,
        reason:
            'the computed function was called an unexpected number of times');
  });

[Yegair]

@rodydavis the PR is ready from my side to be reviewed and discussed (don't want to bother you, just make sure it doesn't go unnoticed 😅)