Bloc: [Proposal] Allow blocs to finish processing pending events on close

[felangel]

Is your feature request related to a problem? Please describe.
Currently, if close is called on a bloc while an event is being processed, the subsequent state changes (transitions) will be ignored.

For example, given the following bloc:

class AsyncBloc extends Bloc<AsyncEvent, AsyncState> {
  @override
  AsyncState get initialState => AsyncState.initial();

  @override
  Stream<AsyncState> mapEventToState(AsyncEvent event) async* {
    yield state.copyWith(isLoading: true);
    await Future<void>.delayed(Duration(milliseconds: 500));
    yield state.copyWith(isLoading: false, isSuccess: true);
  }
}

The current behavior is:

test('close while events are pending does not emit new states', () {
    final expectedStates = <AsyncState>[
        AsyncState.initial(),
    ];
    final states = <AsyncState>[];
    asyncBloc.listen(states.add);

    asyncBloc.add(AsyncEvent());
    asyncBloc.close();
    expect(states, expectedStates);
});

Describe the solution you'd like
It might be better to modify close to allow for any pending events to be processed to completion. This would help with testing and would also allow developers to know when a bloc has been closed as the process is async.

test('close while events are pending finishes processing pending events', () async {
    final expectedStates = <AsyncState>[
        AsyncState.initial(),
        AsyncState.initial().copyWith(isLoading: true),
        AsyncState.initial().copyWith(isSuccess: true),
    ];
    final states = <AsyncState>[];
    asyncBloc.listen(states.add);

    asyncBloc.add(AsyncEvent());
    await asyncBloc.close();
    expect(states, expectedStates);
});

See #611 for more more context and #638 for a draft.

[jorgecoca]

IMO, this makes total sense... but I wonder if there should be a closeWithoutWaiting or similar that disposes immediately... don't know what the use case would be though. Maybe it'd better to wait to hear a request from the community.

[felangel]

IMO, this makes total sense... but I wonder if there should be a closeWithoutWaiting or similar that disposes immediately... don't know what the use case would be though. Maybe it'd better to wait to hear a request from the community.

There is a valid request for this in #611

[tenhobi]

This actually sounds good. It would help tests and it probably would not change current behavior with BlocProvider etc., right?

Let me ask a different question: where don't we want such a behavior?

[felangel]

@tenhobi yup it would have no impact on the functionality of any existing flutter_bloc widgets. I personally can’t think of a scenario where we wouldn’t want this behavior either.

[narcodico]

There should be a configurable flag to enable/disable this. There are situations when you don't want to finish processing an event. e.g.: a user wants to navigate to a different route while the current bloc is processing an event which will result in some new UI state that the user probably doesn't care anymore since he initiated a navigation to a completely new screen. There's no point in waiting on that event and the state to be yielded before closing that bloc.

[felangel]

@RollyPeres in those cases there wouldn’t be any waiting on the event. The only difference is the state change would happen as opposed to right now the logic still executes but when the state change is about to happen it gets ignored. When disposing blocs, BlocProvider would not need to wait for close to complete and if you want to wait you can handle disposing manually and await close yourself. Let me know what you think and thanks for the feedback!

[narcodico]

Or even better, have all events be fully processed by default and have a List<Event> which are safe to be skipped. This is more granular and would give the user more flexibility overall.

[felangel]

What do you mean by have a List? Can you provide some sample code?

[narcodico]

Maybe the bloc could have a List<Type> discardEvents and if you pass in something like discardEvents: <Type>[Increment, Decrement] then you can do something like(semi-pseudo-code):

 if (discardEvents.contains(event.runtimeType)) {
      // discard event
    } else {
      // fully process event
    }

This could probably be improved when dart team will release something like Type.isExactly(event, Increment). Something around these lines is in the works from what I saw at some point.

[felangel]

@RollyPeres thanks for the clarification. I'm trying to better understand under which circumstances would you not want pending events to be processed to completion? In the current state, bloc is not very "predictable" in this regard because depending on timing you might get different outcomes when close is called. If we make the change to always fully process pending events, then as a developer you can be confident of the outcome every time. Thoughts?

[binoypatel]

I agree with @jorgecoca for having closeWithoutWaiting which can be helpful when calling fire-and-forget API or similar

[ThinkDigitalSoftware]

I agree that it should process pending events by default and that if you want it to stop immediately, which I don't see a use case for, you can do, bloc.close(force: true) // defaults to false

It should not have a separate method that does the same thing for simplicity reasons IMO.

[felangel]

@binoypatel do you have a use case you could share where you would want the fire and forget functionality?

[narcodico]

@felangel Say the user is on screen A, initiates an event and then clicks away from it by going to screen B. If the setup is that screen A gets disposed together with the bloc A, and say that event is just some data fetch with no side effects, then what is the point of waiting for that event to finish ? It's just gonna yield a state that the user doesn't care about anymore. I'm sure like 90% of cases revolve around fully processing all the events for predictability as you mentioned, but there's gotta be a couple of niche cases where having the option to skip over some events might be great.

[ThinkDigitalSoftware]

@RollyPeres I'm always in support of those just in case cases

[felangel]

@RollyPeres in those cases the only thing that would change is the bloc would have one additional transition occur. It wouldn’t block the UI or navigation and currently the bloc still finishes processing events but when it comes time to emit the new states they are all ignored so it wouldn’t introduce any additional overhead. Let me know if that helps clarify things.

[narcodico]

@ThinkDigitalSoftware finally, a believer 😂
@felangel your proposal is fine to be honest, I've nothing against it. Definitely, it's the most predictable way of doing it. But that doesn't mean there aren't some edge cases. What about an event which might add new events to the sink, while the bloc is not finished processing this event?

[felangel]

@RollyPeres in that case those events will not be added because no matter what once close is called no new events can be added. I completely agree with you but I'd like to have an understanding of concrete edge cases instead of speculating. So far I can't think of any cases where the current implementation would be desirable over the proposed implementation.

[ThinkDigitalSoftware]

@RollyPeres @felangel It would make sense to me if it behaved the same way stores do when they close. Don't allow in new customers, but allow the existing customers to complete their checkout. I would say let the bloc finish it's own internal processing, even if that includes adding another event to the stream, but I don't see how that would be able to be controlled

[felangel]

@ThinkDigitalSoftware I don't think we should allow new events to be added. Wouldn't that be like existing customers letting in their friends after the store has closed?

[ThinkDigitalSoftware]

It would be like parents having their children checkout separately

[ThinkDigitalSoftware]

it's like a Future than has a then clause. Except in bloc, you either process the "then" inside the same run, or dispatch an event if you already have code set up to respond to that event.

[felangel]

@ThinkDigitalSoftware I think those are different because the bloc shouldn't care where the event came from in my opinion. We shouldn't be differentiating between internal events and external events.

[felangel]

It would be like parents having their children checkout separately

I disagree because with the children analogy, the children would already be in the store (events were already added). In the case you're describing we'd be adding new events.

[ThinkDigitalSoftware]

I agree that it wouldn't be able to differentiate easily, but it seems like if someone happened to use that pattern it may take some debugging to figure out why their event didn't process. I guess an error thrown if an event was added after it was closed would work, but customized for bloc

[felangel]

@ThinkDigitalSoftware there is already an error thrown in onError when events are added after the bloc is closed. Don't you think the documentation is pretty clear on this already?

Once close is called, events that are added will not be processed and will result in an error being passed to onError.

https://pub.dev/packages/bloc

[ThinkDigitalSoftware]

I

It would be like parents having their children checkout separately

I disagree because with the children analogy, the children would already be in the store (events were already added). In the case you're describing we'd be adding new events.

I was thinking more like the kids were attached to their parents and came in at once, but then things changed. With bloc extending stream, this no longer makes sense since an add call externally is the same as one internally

[ThinkDigitalSoftware]

@ThinkDigitalSoftware there is already an error thrown in onError when events are added after the bloc is closed. Don't you think the documentation is pretty clear on this already?

Once close is called, events that are added will not be processed and will result in an error being passed to onError.

https://pub.dev/packages/bloc

Oh, then yes, it's perfect :) Or if you wanted to go a step further, you could mention the event that was added, just in case someone uses this pattern. I've seen many a bug happen because of some forgotten side effect. But that's just extra protection against bad programming behaviors

[binoypatel]

Basically, all long running / background task are good fit for fire-and-forget, for example, user set settings for importing active directory users (let say there are more than 10,000 users) and then click on import button on the screen, and then navigate to the home page, import will trigger fire-and-forget api which will take long time to complete and not require user to wait until the process is finished

[felangel]

@binoypatel sorry if I was confusing but none of the proposed changes would force a user to wait until a process is finished. All bloc processing is happening asynchronously and would not block the UI. Furthermore in your example, if close is called in the middle of an import would you want the import to be partially complete? I would imagine you wouldn't want your app to be in a state where an import was partially executed (this is what I was referring to when I said the current implementation is not predictable). Instead, the proposal would allow for the bloc to finish the pending import asynchronously while the user navigates to the home page.

[binoypatel]

Thanks for the details, It is more clear now, I think in this case it will make sense to await before closing but then the caller will be forced to be async'ed which may introduce lot of changes for consumer

[felangel]

@binoypatel no problem! You would only need to make changes if you're overriding close in your bloc like:

@override
void close() {
  _subscription.cancel();
  super.close();
}

You'd need to change to:

@override
Future<void> close() {
  _subscription.cancel();
  return super.close();
}

You wouldn't need to await and for the majority of cases there should be no change necessary since BlocProvider handles calling close 👍

[felangel]

Merged in #638 and will be included in v2.0.0 of bloc.

[rrousselGit]

IMO it doesn't make sense to emit things after the stream is closed. At best it's hacky, at worse it cause undesired effects.

That's why try/finally is there IMO:

() async* {
  print('start');
  try {
    await Future.delayed(const Duration(seconds: 5));
    yield MyValue();
  } finally {
    print('finished');
  }
}

The finally clause will _always_ be executed. In such snippet, even if the stream is closed during the Future.delayed, it will still print "finished", even if yield MyValue() isn't.

[felangel]

@rrousselGit we wouldn't be emitting things after the stream is closed. The proposal was to close the sink, finish processing/emitting any pending events, and then close the stream. This would ensure that if an event is added to the bloc before close is called, the event would be handled to completion rather than leaving the bloc in a non-deterministic state (it may or may not have finished processing the event depending on how long things took).

Edit: I saw your response on twitter and it looks like we're in agreement 😄