[proposal] feat(bloc): cancelable async operations

[felangel]

Description

As a developer, I want to be able to await asynchronous operations within a bloc/cubit which are automatically canceled if the instance is closed while the async operation is pending.

An example use-case is when using a cubit to fetch some data asynchronously from a screen in which a user can go back.

static Route<MyScreen> route() {
  return MaterialPageRoute(
    builder: (context) => BlocProvider(
      create: (context) => MyCubit()..load(),
      child: const MyScreen(),
    ),
  );
}

enum MyState { idle, loading }

class MyCubit extends Cubit<MyState> {
  MyCubit() : super(MyState.idle);

  Future<void> load() async {
    emit(MyState.loading);
    await Future<void>.delayed(const Duration(seconds: 3));
    emit(MyState.idle);
  }
}

In this scenario, as soon as MyScreen is pushed onto the navigation stack, the load() method is called on a newly created instance of MyCubit. It's possible that the user might get tired of waiting and press the back button before load() has completed. In this case, the Future will still complete after the MyCubit has been closed and the subsequent emit(MyState.idle) will be evaluated which will result in a StateError:

Unhandled Exception: Bad state: Cannot emit new states after calling close

Desired Solution

It would be nice if we had a cancelable (open to naming suggestions) API which allowed developers to await asynchronous operations which would automatically be canceled if the bloc/cubit was closed.

enum MyState { idle, loading }

class MyCubit extends Cubit<MyState> {
  MyCubit() : super(MyState.idle);

  Future<void> load() async {
    emit(MyState.loading);
    await cancelable(() => Future<void>.delayed(const Duration(seconds: 3)));
    emit(MyState.idle);
  }
}

Alternatives Considered

• Developers could check if the instance has been closed after any asynchronous operations and abort:

enum MyState { idle, loading }

class MyCubit extends Cubit<MyState> {
  MyCubit() : super(MyState.idle);

  Future<void> load() async {
    emit(MyState.loading);
    await Future<void>.delayed(const Duration(seconds: 3));
    if (isClosed) return;
    emit(MyState.idle);
  }
}

• Developers could also use CancelableOperation and CancelableCompleter from package:async to maintain a list of cancelable operations internally which are manually canceled when the instance is closed.

• emit could automatically ignore states after the instance has been closed (previous behavior)

Additional Context

See #2980 and #3042.

[felangel]

cc @jorgecoca @orestesgaolin @marcossevilla @erickzanardo

[DenisBogatirov]

Hey, @felangel approach with cancelable(() => ...) seems, ok. I've tried implementing it myself and it seems like not much code, but it would be a very nice feature to have out of the box and "marked" as "approved approach".

Also it should be present on Bloc (or BlocBase) as well, because the same scenario is appearing using Bloc.

Here is my rough implementation of your idea as mixin

mixin CancelableBaseBloc<T> on BlocBase<T> {
  final _cancelables = <CancelableOperation>[];

  Future<K> cancelable<K>(Future<K> Function() future) {
    final operation = CancelableOperation.fromFuture(future());

    _cancelables.add(operation);

    return operation.value;
  }

  @override
  Future<void> close() async {
    for (final cancelable in _cancelables) {
      cancelable.cancel();
    }
    super.close();
  }
}

[erickzanardo]

Both proposals LGTM, I slight lean towards the first one with the cancelable method.

I wonder if this shouldn't also exists on Bloc? I believe they are prone to the same issue?

[DenisBogatirov]

Yes, same scenario is appearing using Bloc. So the solution discussed here should exist on Bloc as well.

[DenisBogatirov]

Hey, @felangel approach with cancelable(() => ...) seems, ok. I've tried implementing it myself and it seems like not much code, but it would be a very nice feature to have out of the box and "marked" as "approved approach".

Also it should be present on Bloc (or BlocBase) as well, because the same scenario is appearing using Bloc.

Here is my rough implementation of your idea as mixin

mixin CancelableBaseBloc<T> on BlocBase<T> {
  final _cancelables = <CancelableOperation>[];

  Future<K> cancelable<K>(Future<K> Function() future) {
    final operation = CancelableOperation.fromFuture(future());

    _cancelables.add(operation);

    return operation.value;
  }

  @override
  Future<void> close() async {
    for (final cancelable in _cancelables) {
      cancelable.cancel();
    }
    super.close();
  }
}

mixin is used just so I will not modify sources :)

[jolexxa]

Developers could also use CancelableOperation and CancelableCompleter from package:async to maintain a list of cancelable operations internally which are manually canceled when the instance is closed.

Wouldn't this approach create the most readable code? That is, I (subjectively) think this would be the better approach in many situations.

Think of blocs which utilize event concurrency — wrapping everything in cancelable() convenience methods does not reveal the order in which operations are canceled, because the order that events are processed in is non-linear as of Bloc>=7.2.

If you added all your cancelable operations to a queue, as you described, and then cancel them during the bloc's close() method, the order in which asynchronous operations are canceled is far more apparent (although requires more effort). I would worry that cancelable() would be abused in the concurrent-event-processing-by-default world.

Then again, maybe it's a non-issue, depending on how canceling futures works under the hood.

[felangel]

Developers could also use CancelableOperation and CancelableCompleter from package:async to maintain a list of cancelable operations internally which are manually canceled when the instance is closed.

Wouldn't this approach create the most readable code? That is, I (subjectively) think this would be the better approach in many situations.

Think of blocs which utilize event concurrency — wrapping everything in cancelable() convenience methods does not reveal the order in which operations are canceled, because the order that events are processed in is non-linear as of Bloc>=7.2.

If you added all your cancelable operations to a queue, as you described, and then cancel them during the bloc's close() method, the order in which asynchronous operations are canceled is far more apparent (although requires more effort). I would worry that cancelable() would be abused in the concurrent-event-processing-by-default world.

Then again, maybe it's a non-issue, depending on how canceling futures works under the hood.

I could go either way on this. Using CancelableOperation and CancelableCompleter would be the most explicit way to handle this; however, it requires developers to create, track, and cancel the list of operations manually which is quite repetitive. In addition, it requires an additional dependency on package:async.

In terms of how cancelable would work, I envisioned that operations would be added to a queue and canceled in order with respect to when it was registered within the specific event handler (as you mentioned). I don't think it would have much of an effect with regards to bloc_concurrency because transformers apply to the event handler which takes priority over a specific cancelable operation. If an event handler is canceled all corresponding cancelable operations should also be canceled for that particular event handler.

The main goal is to have an explicit way to cancel pending async operations when a bloc/cubit is closed -- I'm totally open to any other suggestions if you have them 👍

[maRci002]

  Future<void> load() async {
    emit(MyState.loading);
    await cancelable(() => Future<void>.delayed(const Duration(seconds: 3)));
    emit(MyState.idle);
  }

If this or similar will be the implementation:

mixin CancelableBaseBloc<T> on BlocBase<T> {
  final _cancelables = <CancelableOperation>[];

  Future<K> cancelable<K>(Future<K> Function() future) {
    final operation = CancelableOperation.fromFuture(future());

    _cancelables.add(operation);

    return operation.value;
  }

  @override
  Future<void> close() async {
    for (final cancelable in _cancelables) {
      cancelable.cancel();
    }
    super.close();
  }
}

Then this might cause memory leaks since operation.value never will be returned (if operation was canceled before it completes) and load function will just hung in memory forever.

[felangel]

  Future<void> load() async {
    emit(MyState.loading);
    await cancelable(() => Future<void>.delayed(const Duration(seconds: 3)));
    emit(MyState.idle);
  }

If this or similar will be the implementation:

mixin CancelableBaseBloc<T> on BlocBase<T> {
  final _cancelables = <CancelableOperation>[];

  Future<K> cancelable<K>(Future<K> Function() future) {
    final operation = CancelableOperation.fromFuture(future());

    _cancelables.add(operation);

    return operation.value;
  }

  @override
  Future<void> close() async {
    for (final cancelable in _cancelables) {
      cancelable.cancel();
    }
    super.close();
  }
}

Then this might cause memory leaks since operation.value never will be returned (if operation was canceled before it completes) and load function will just hung in memory forever.

I don't think we need to worry about implementation details in this issue. I feel it would be best if we could all just focus on defining the API/behavior/usage. Once we align on that we can discuss implementation details 👍

[maRci002]

I have another situation which might benefit from cancel token so it's related to this issue when defining behaviors:

class TestEvent {
  final String id;
  TestEvent(this.id);
}

abstract class TestState {}

class TestInitial extends TestState {}

class TestLoading extends TestState {
  final String id;
  TestLoading({required this.id});

  @override
  String toString() => 'TestLoading{id: $id}';
}

class TestLoaded extends TestState {
  final String id;
  final Object result;
  TestLoaded({required this.id, required this.result});

  @override
  String toString() => 'TestLoaded{id: $id}';
}

class TestBloc extends Bloc<TestEvent, TestState> {
  final _random = Random();

  TestBloc() : super(TestInitial()) {
    on<TestEvent>(_onTestEvent);
  }

  Future<void> _onTestEvent(TestEvent event, Emitter<TestState> emit) async {
    final id = event.id;
    emit(TestLoading(id: id));
    // result which is calculated by id so someResult is related only to specific id
    var someResult = await Future<void>.delayed(Duration(seconds: _random.nextInt(10)));
    emit(TestLoaded(id: id, result: id));
  }
}

Imagine a situation when some event happens: context.read<TestBloc>().add(TestEvent('0')); // load data by id: 0 Soon another event happens (we are no more aware of '0' event's states so it should be canceled): context.read<TestBloc>().add(TestEvent('1')); // load data by id: 1

However currently we cannot cancel event '0', the output might look like this:

TestBloc Change { currentState: Instance of 'TestInitial', nextState: TestLoading{id: 0} }
TestBloc Change { currentState: TestLoading{id: 0}, nextState: TestLoading{id: 1} }
TestBloc Change { currentState: TestLoading{id: 1}, nextState: TestLoaded{id: 1} }
TestBloc Change { currentState: TestLoaded{id: 1}, nextState: TestLoaded{id: 0} }

[narcodico]

@maRci002 right now you can make use of restartable() from bloc_concurrency to cancel your event handler and only process the latest.

[maRci002]

@narcodico thanks it looks promising, I will take a look. Only problem Cubits doesn't benefit from this.

[DenisBogatirov]

I can think of two other possible solutions ( may not be great thou :) )

1. Is additional param to emit, something like ignoreBadState: false // by default. Passing true will behave like previous implementation.
2. Allow override emit behavior (or override emit's check behavior) through BlocOverrides.

[jeroen-meijer]

I think this would be an awesome addition.

Does this mechanic in its current proposed implementation work (or would it make sense to see if we can make it work) such that we don't even fully await the operation, but immediately cancel the Future when we don't need it anymore (instead of waiting for it to finish and then not doing anything with the result)?

[Gene-Dana]

What about a mixin? CancellableBloc

[cmc5788]

Personally, I'm feeling like it might be best to just bring back the behavior of ignoring emit-after-close as the default behavior.

I can definitely see the value in throwing runtime errors if events are added after close, but for internal async work to throw runtime errors if emit happens after close, it feels like there's currently no good option to make this "more good than harm."

Other frameworks have solved similar problems by allowing a check for isClosed prior to emit, or adding a variant of emit (maybeEmit ?).

However, if it's up to the developer to remember to do this without any static analysis checks to help them out, I expect that a lot of buggy apps will be released to production, because the behavior isn't "consistent" and may only be exposed under certain race conditions, etc.

I can see it being good to add this strict check back as a developer opt-in behavior, or maybe make it the default if dartlang updates in the future make it possible to add static analysis rules in a more developer-friendly way.

For example it would be really cool if;

Future<void> load() async {
  emit(MyState.loading);
  await Future<void>.delayed(const Duration(seconds: 3));
  emit(MyState.idle); // Analyzer warning: don't "emit" after "await" without checking "isClosed"
}

edit: or, maybe just make the runtime error into an assert so it doesn't blow up apps in production?

[raulmabe]

edit: or, maybe just make the runtime error into an assert so it doesn't blow up apps in production?

From my point of view this solution would be the best for all parties.

• We'll get the benefits from letting us know if a bloc has emitted an state after it was closed to prevent bugs.
• No need to migrate apps using previous versions
  • Not having to add isClosed checks, nor cancelable wrappers to every network requests we do (which ain't a few)

[felangel]

@cmc5788 @raulmabe thanks for the feedback!

The reason I'm hesitant to switch to using an Assertion is because I don't think it would make a difference. Currently a StateError is thrown but in both cases the blocs will continue to function after the error and I would guess developers would still not like to have assertions being thrown randomly in their applications (unless they check isClosed before calling emit).

I am leaning towards just reverting this behavior to just ignore emitted states if the instance is already closed.

Let me know what you think and thanks again for the feedback 👍

[cmc5788]

@cmc5788 @raulmabe thanks for the feedback!

The reason I'm hesitant to switch to using an Assertion is because I don't think it would make a difference. Currently a StateError is thrown but in both cases the blocs will continue to function after the error and I would guess developers would still not like to have assertions being thrown randomly in their applications (unless they check isClosed before calling emit).

I am leaning towards just reverting this behavior to just ignore emitted states if the instance is already closed.

Let me know what you think and thanks again for the feedback 👍

Just to play devil's advocate: from the standpoint of designing a solution to be as efficient as possible, as a developer, you ideally want to check isClosed prior to any expensive async operation.

Even if the result is ignored harmlessly, the expensive part is the actual async task itself, which often involves network or database usage, parsing, possibly communicating across isolates. Or possibly even doing the work has some kind of side effect like a login API call that caches the session. To do all of that and then throw away the result isn't ideal, but whether to treat something that "might" indicate that a developer missed that kind of problem as an error or not feels like kind of a personal decision 😂

Whatever the mechanism for it happens to be, I think it's useful as a developer to have the option of being warned when you might have missed an opportunity to avoid a potentially expensive async operation. Maybe the default of throwing an error was too annoying, but I'm also not sure ignoring it with no option to make it visible is the right solution 🤔

I think in terms of priorities of behaviors with 1 being my personal favorite I'd do something like --

1. Ignore it, somehow let developers opt into the error behavior
2. Treat it like an assert
3. Ignore it, no configuration
4. Treat it like an error

But it's not a strong opinion either way, since developers have the tools they need to make it work regardless.

[narcodico]

I feel bloc has done a great job so far not polluting the library with excessive and unneeded configurations. But in this situation we might consider adding a configuration on the BlocOverrides, which would default to previous behavior of ignoring states once the controller is closed, but allow for developers into red messages to opt in.

[raulmabe]

Hi :wave:

How is the state of this issue? I am currently feeling obliged to constantly check isClosed before emitting new states on all blocs, which is a bit tedious...

[nosmirck]

Hi 👋

How is the state of this issue? I am currently feeling obliged to constantly check isClosed before emitting new states on all blocs, which is a bit tedious...

I use Cubits and this extension has come in handy :) hope it helps!

extension CubitExt<T> on Cubit<T> {
  void safeEmit(T state) {
    if (!isClosed) {
      // ignore: invalid_use_of_visible_for_testing_member, invalid_use_of_protected_member
      emit(state);
    }
  }
}

[kaciula]

@felangel What is the status of this issue? Do you plan to revert the default behavior to ignore the emitted states if the instance is already closed? I need to know if I should update ALL my emit calls in my apps or I should wait for a package update. I'm getting crash reports in the wild and I need to do something about it.

[maRci002]

If developers wants to decide to ignore StateError or not then they should config it by globally per bloc/cubit #3042 and/or handle it locally by emit(state, ignoreStateError: true) / safeEmit(state) / maybeEmit(state).

Personally I like the StateError because I am forced to eliminate expensive async operations as well when bloc/cubit already closed.

However it would be even better if analyzer could help and remove StateError completely.

This can be achived by using @useResult annotation on emit and it should return bool instead of void indicating wheather emit(state) was able to emit or not and remove throw StateError.

  /// Emits the provided [state].
  @UseResult('Returns `false` if Bloc/Cubit is already closed so abort your function like this: `if (!emit(state) return;)`')
  bool call(State state);

class CounterCubit extends Cubit<int> {
  CounterCubit() : super(0);

  // non nesting version
  Future<void> increment1() async {
    if (!emit(0)) return;
    await Future<void>.delayed(const Duration(seconds: 3));
    final _ = emit(1);
  }

  // nested version
  Future<void> increment2() async {
    if (emit(0)) {
      await Future<void>.delayed(const Duration(seconds: 3));

      final _ = emit(1);
    }
  }
}

final _ = emit(1); looks bad however if (!emit(1)) return; can be also used as a habit.

If emit(state)'s result isn't used the following analyzer warning pops up:

The value of 'call' should be used. Try using the result by invoking a member, passing it to a function, or returning it from this function. dart (unused_result)

Unfortunetly @useResult annotation doesn't work if it is invoked on a callable class' instance (I made an issue dart-lang/sdk#48913) unless it is invoked via .call() syntax e.g.: emit.call(0).

edit: from dart 2.18 @useResult annotation works on callable method.

[lirantzairi]

@cmc5788 @raulmabe thanks for the feedback!

The reason I'm hesitant to switch to using an Assertion is because I don't think it would make a difference. Currently a StateError is thrown but in both cases the blocs will continue to function after the error and I would guess developers would still not like to have assertions being thrown randomly in their applications (unless they check isClosed before calling emit).

I am leaning towards just reverting this behavior to just ignore emitted states if the instance is already closed.

Let me know what you think and thanks again for the feedback 👍

@felangel This would be the best approach in my opinion because it'll resemble the behavior of Flutter's FutureBuilder or StreamBuilder - these widgets check if they have been disposed of prior to calling the builder function. Moreover, developers won't need to make changes to their code in order to use it. I'd love to get rid of all the dozens of if (isClosed) return; lines in my code :) Thanks

[bartekpacia]

I like the current behavior of throwing if emit() is called when bloc/cubit is already closed, because it forces you to think about the fact that your async work isn't canceled after the user eg. exits the screen with that bloc/cubit. In most cases, the maybeEmit() extension method on BlocBase works just fine:

extension CubitMaybeEmit<S> on Cubit<S> {
  @protected
  void maybeEmit(S state) {
    if (isClosed) {
      return;
    }

    // ignore: invalid_use_of_protected_member, invalid_use_of_visible_for_testing_member
    emit(state);
  }
}

I'd like also like to bring up the topic that I feel nobody has brought up in this thread, that is: Futures in Dart aren't preemptive. Citing that issue, a Future is not a computation, but the result of that computation. So it's not possible to cancel an async operation, as the title of this issue says.

[denisano]

I like the current behavior of throwing if emit() is called when bloc/cubit is already closed, because it forces you to think about the fact that your async work isn't canceled after the user eg. exits the screen with that bloc/cubit. In most cases, the maybeEmit() extension method on BlocBase works just fine:

extension CubitMaybeEmit<S> on Cubit<S> {
  @protected
  void maybeEmit(S state) {
    if (isClosed) {
      return;
    }

    // ignore: invalid_use_of_protected_member, invalid_use_of_visible_for_testing_member
    emit(state);
  }
}

I'd like also like to bring up the topic that I feel nobody has brought up in this thread, that is: Futures in Dart aren't preemptive. Citing that issue, a Future is not a computation, but the result of that computation. So it's not possible to cancel an async operation, as the title of this issue says.

We are using similar way and it works fine. It would be good to have the method maybeEmit right in the bloc package.

[rishabhsharma3617]

@felangel i tried the solution by you and @DenisBogatirov by using a mixin class for CancelableOperation. But it seems out that even CancelableOperation doesn't work as expected , in my code i was still executing the future and code post it even after closing it properly on bloc close , which is leading to the StateError , right now i am making it work by forking the bloc package to my internal server and not throwing the error .

But see the issue opened for Cancellable Operation here link

Also i would love to hear any more solutions from the community

[rumax]

I like the current behavior of throwing if emit() is called when bloc/cubit is already closed, because it forces you to think about the fact that your async work isn't canceled after the user eg. exits the screen with that bloc/cubit. In most cases, the maybeEmit() extension method on BlocBase works just fine:

extension CubitMaybeEmit<S> on Cubit<S> {
  @protected
  void maybeEmit(S state) {
    if (isClosed) {
      return;
    }

    // ignore: invalid_use_of_protected_member, invalid_use_of_visible_for_testing_member
    emit(state);
  }
}

I'd like also like to bring up the topic that I feel nobody has brought up in this thread, that is: Futures in Dart aren't preemptive. Citing that issue, a Future is not a computation, but the result of that computation. So it's not possible to cancel an async operation, as the title of this issue says.

@felangel Why not to implement it inside bloc and allow to have both emit and maybeEmit (or safeEmit)? without need to write extension by each developer and will make everyone happy.

In most cases doing a check for isClosed is over complication of the code. Just think about a bit more complicated code with more that 1 await:

Future<void> load() async {
    emit(MyState.loading);

    final id = await getIdOfDetails();

    if (!id) {
      emit(ErrorState);
      return
    }

    final details = await loadMoreInfo(id);

     if (!details) {
      emit(ErrorState);
      return
    }

    emit(details ? SuccessState(details) : Error);
  }

With proposal to do isClosed the code will be

Future<void> load() async {
    emit(MyState.loading);

    final id = await getIdOfDetails();

    if (isClosed) { // Otherwise cannot emit error state
      return;
    }

    if (!id) {
      emit(ErrorState);
      return
    }

    final details = await loadMoreInfo(id);

    if (isClosed) { // Otherwise cannot emit error state or success state
      return;
    }

     if (!details) {
      emit(ErrorState);
      return
    }

    emit(details ? SuccessState(details) : Error);
  }

what is the benefit of it?

[SAGARSURI]

Looking forward to this change.