Improve dispose while events are being processed

[felangel]

Is your feature request related to a problem? Please describe.
It is fairly common to call dispose on a bloc while there are still events being processed and states are being yielded. As a result, an error is caught by the BlocDelegate which looks something like:

Unhandled Exception: Bad state: Cannot add new events after calling close

Describe the solution you'd like
It'd be nice to be able to ensure that a bloc does not have any pending events before calling dispose so that you can close the streams when they are empty.

I would love feedback regarding which approach people prefer:

Approach 1:
Ignore all states that are being processed after dispose is called and completely handle it internally within the bloc.

As a developer, there would be no impact. The API would stay exactly the same.

Approach 2:
Automatically drain the events and states in the streams before calling dispose.

As a developer, the impact would be code that looked like

@override
void dispose() {
  bloc.dispose();
  super.dispose();
}

Would have to change to look like

@override
void dispose() async {
  await bloc.dispose();
  super.dispose();
}

Approach 3:
Add a drain API to bloc so that developers have the option to drain the streams before calling bloc.dispose.

As a developer, the impact would be code that looked like

@override
void dispose() {
  bloc.dispose();
  super.dispose();
}

Could be changed (optional)

@override
void dispose() async {
  await bloc.drain();
  bloc.dispose();
  super.dispose();
}

Additional context
Referenced in

• Bad state: Cannot add new events after calling close after the bloc is disposed. #120
• Bad state: Cannot add new events after calling close in BottomNavigationBar #256

[smiLLe]

IMO we should just ignore all future states because this is what i expect to happen if i bloc.dispose();.

[dev-aritra]

I think the 3rd option would be the best, logically we should be doing the 1st one, as that is what the method name suggests and what it should be, but as a developer, we could always be making mistakes or some exceptional situations may arise, like if there are few events which are still happening which are not supposed to be happening, if you directly close it, we won't know which events were happening and why were those happening. So I feel giving the developer the control over it would be the best UX, but the option 1 should suffice for most of the situations though.

[felangel]

@dev-aritra With Option 1, you would have to keep in mind that as soon as you call bloc.dispose() no additional processing will happen (no state changes/transitions). Can you please give a bit more detail about why you would want to know that an event is still being processed?

Currently, if you call dispose and there are events pending you will see errors in the onError callback of the bloc and delegate. From the feedback I've received, it seems like developers assumed that dispose would ignore all pending/new events so they opened issues when they saw these errors.

Also as a developer, you have control over when dispose is called so you can wait for certain events to have been processed before calling dispose if you really want to. I would love to hear more about why you feel there is a lack of control with Option 1 vs Option 3.

Thanks so much for the feedback; I really appreciate it!

[dev-aritra]

@felangel For the first question, I will try to come up with some example scenario, like your app is receiving different events (BROADCAST RECEIVERS for android), so you don't have control over when these events happen, we could argue over the fact that we need to add these event handles on a global bloc level rather than a bloc for a specific page, but that's a different discussion. Same goes for actions like online-offline data sync. So for these kinds of scenarios I might wanna know what actually went down.

It might be a very personal choice, but I like having a try-catch instead of a onError callback. If my assumptions are correct (really not sure whether it's correct or not) I can do try-catch on the 3rd option and I have to depend upon onError on the first option, please let me know if I assumed it right.

[axellebot]

The third option seems better to me for 2 reasons :

• dispose() methods (as I know) are mainly sync in flutter (maybe also dart and angular)
• Wait the end of the process should be an options and not a default behavior

You can maybe add a optional dryBeforeDispose (false by default) parameter to the dispose method but it will maybe disturb as the await statement ?

[felangel]

@axellebot I'm leaning toward option 1 because, like you said, dispose should be sync and so far for all cases I can think of it would make most sense (and be the simplest) to just stop processing all pending events. I think it would be a good place to start and if issues arise we can always circle back and figure out how to accommodate them. Thoughts?

@dev-aritra Like you said, I would argue that in those scenarios the bloc should not be disposed in the sub-tree(since they all sound like global blocs). Regarding the try/catch you would still depend on onError for any exceptions thrown within the bloc.

[felangel]

Merged in #262 and will be included in bloc v0.13.0 🎉

[narcodico]

I agree dispose should be sync. But reactive programming should be predictable right? Bloc is built on top of Streams/Observables. If you just dispose the bloc while async events are being processed you kinda lose the predictable part. An async event might get the chance of being processed or not. To make the best of both worlds, I think you should handle it internally and ignore all pending events at the time of dispose call. This should however not throw an error.

If developers need async dispatched events to be completed or produce a new state then maybe they are not architecting the right way. To me this looks more like you want to keep alive your widget together with your bloc or more of a global bloc that ensures all events are processed. I believe this scenario(the dispose error) occurs a lot when people use bottom nav bar or tabs. This is actually a really good candidate for keeping alive your tab pages. Users will navigate a lot between them, so I'm sure you don't want to reload data each and every time that happens. Keeping them alive will also ensure all events are processed even if the user switches tabs while an event is under processing. And the user will come back to where the screen was left at.