fix: BlocListeners fire for non-active pages that are still in the navigator stack

[mattalbr]

Description
IMO counter-intuitively, BlocListeners do not simply apply to the current route. Even after doing a Navigator.pushNamed, BlocListeners will continue to fire as long as the route that created them is somewhere in the Navigator stack.

This primarily (but not exclusively) comes up when using global blocs -- for us it's happened multiple times with our UserBloc that gets loaded both on initial login, and reloaded on various occasions. For example, the first page of our app with a sign in button has a BlocListener that waits for UserBloc to reach a logged in state and navigates to the next screen of our account creation flow. We then later reload the UserBloc after account creation is finished, which confusingly triggered the listener and re-navigated to the account creation flow.

Obviously there are ways for us to work around this specific issue, but from our perspective this is a huge footgun that has caused us multiple bugs in production, and is extremely challenging to debug. We're finally going to apply a big hammer and check in every BlocListener's listenWhen for (ModalRoute.of(context)?.isCurrent ?? false) but IMO this is something that should be handled by BlocListener itself.

This has come up in the past via #2255 and #2575 but from what I can tell, both of those bugs got closed after finding workarounds rather than addressing the core issue that this counter-intuitive behavior is bug prone and warrants re-consideration.

Steps To Reproduce

1. Create an app with BlocListener<FooBloc, FooState>
2. Call Navigator.pushNamed(Routes.bar)
3. From Routes.bar, fire an event that pushes a new state to FooBloc's stream
4. Observe that the original BlocListener fires again

Expected Behavior
BlocListeners should only apply to the current page, i.e. when (ModalRoute.of(context)?.isCurrent ?? false) is true

[mattalbr]

Another terrifying datapoint:
We believe that we're seeing this behavior even when using pushReplacementNamed, specifically when running on iOS or the iOS simulator, but not 100% of the time. I'm assuming that the BlocListeners run until the Widget gets disposed, and there's probably some randomness or optimization happening behind the scenes, possibly even platform dependent, that exacerbates this issue.

[mattalbr]

Another possible fix that I will try out this week or next is to pause the stream subscription in BlocListener in deactivate and resume it in activate. I believe this is the proper fix, vs something like ModalRoute.of(context)?.isCurrent

[tenhobi]

I understand how unintuitive it might seem, but mobile it is made different from i.e. web. On web you always have just one screen and the others are gone. In Flutter (mobile in general), because of all the features like predictive back when you can actually see the previous route when doing the back animation etc., all of the routes in your navigation stack are being there and of course being active in some way or another. Imagine it like sheets of papers one above another -- you still have your notes and drawings there.

That being said, for the same reason, stuff like BlocListener and in general any thing that reacts to something, is working as it is supposed to work. If you have a global bloc on multiple routes in one navigation stack and it does mischief, that is unfortunately lack of bad app-design. These blocks might be for example created using factory, not singleton (global), the blocs should not have a memory (I mean in bloc, obviously they have states) etc. Or you can also set up better concurency transformer https://pub.dev/packages/bloc_concurrency.

But of course if you still want to keep it your way, it's completely fine to use ModalRoute.of(context)?.isCurrent yourself to deal with the inproper design/use.

[mattalbr]

While the example I gave was with a global bloc, this does not at all require such a situation -- it doesn't even require sharing the bloc between pages. See, for example, production of this bug via pushReplacementNamed which completely removes the page off of the stack.

The fact that the pages remain in memory somewhere in mobile stacks like flutter is, IMO, an implementation detail (primarily a performance optimization) and not something to design an application layer API contract around. Flutter intentionally gives us a deactivate API for exactly this reason because without it, the developer has absolutely no control over when a route is disposed. I cannot see how pushing a replacement route and having a listener trigger while the old route is popped off the stack but in a weird mostly dead state is anything other than a bug.

At the end of the day the choice is yours, but from my perspective having the framework that we use to orchestrate our application logic contain unintuitive, racy and platform dependent behavior is a recipe for disaster.

As a contrived example to illustrate my point, imagine such an app:

abstract class NeedleState {}

class NeedleUninitialized extends NeedleState {}

abstract class NeedleInitialized extends NeedleState {}

class CleanNeedle extends NeedleInitialized {}

// TODO(overworked administrator): Reorganize this shelf...
class PoisonTippedNeedle extends NeedleInitialized {} 

abstract class NeedleEvent {}

class GrabNeedle extends NeedleEvent {}

class NeedleBloc extends Bloc<NeedleEvent, NeedleState> {
  NeedleBloc() : super(const NeedleUninitialized());

  void onGrabNeedle(Login event, Emitter<NeedleState> emit) async {
    emit(CleanNeedle());

    // Just a gag really to get some laughs from our patient, our view only ever uses the first needle returned anyway.
    emit(PoisonTippedNeedle());  
  }

  // Transformer isn't relevant since we only ever have one event.
  on<GrabNeedle>(onGrabNeedle);  
}

class ExamRoom extends StatelessWidget {
  const ExamRoom({super.key});

  @override
  Widget build(BuildContext context) {
    return BlocProvider(
      create: (context) => NeedleBloc()..add(GrabNeedle),
      child: _ExamRoom(),
    );
  }
}

class _ExamRoom extends StatelessWidget {
  const  _ExamRoom({super.key});

  @override
  Widget build(BuildContext context) {
    return BlocConsumer<NeedleBloc, NeedleState>(
      listener: (context, state) {
        if (state is NeedleInitialized) {
          AdministerInjection();

          // Our Doctors been known to play some unsettling practical jokes,
          // so let's make sure after we administer an injection we usher the
          // patient out right away.
          //
          // pushReplacementNamed pops our view off the stack synchronously,
          // so we can be sure that this view is no longer in the navigator stack
          // by the time it returns.
          pushReplacementNamed(context, Routes.frontDesk);
        }
      },
      builder: (context, state) => PleasantMedicalDecor(),
    );
  }
}

Now, when running our widget tests, the patient gets a vaccine with a clean needle and ushered to the front desk without incident. When we do extensive QC (primarily web-based, because of the ease of it being platform independent), everything works as intended. When we do our final on-device QC, we get (un)lucky, we win the race condition and everything works as intended.

Then, when we ship this to production, every 10th patient or so gets a vaccine with a clean needle, ushered to the front desk, and while scheduling their follow-up appointment the doctor runs out of the exam room and injects them with the poison tipped needle, even though the only code we have involving injections is for a page that is not even on the stack, or in this analogy we only ever do injections in the ExamRoom, and the patient is confirmed to no longer be in the ExamRoom and will never return to it.

I know the poison tipped needle example is contrived, but I can confirm we have had multiple production incidents directly caused by this behavior, so it does happen in practice, it's just much harder to understand and communicate when ripping code out of a large codebase with no context.

By having such an API contract, it becomes impossible to reason about navigation BlocListener logic because you have no idea what listeners might still be hanging around wreaking havoc. The point isn't that you can't write apps that work around this behavior, it's that when designing a framework it's just as important to understand the developers who will use it's tendencies and expectations even when those are wrong. The flutter team themselves are an excellent example of this -- they deprecated markNeedsBuild in favor of setState because through their UX research they found that users consistently used it wrong, calling it at random unnecessary times, rather than just say "people are using it wrong, Working As Intended".

Thank you for considering this proposal and for all the work you put into this library -- we really appreciate this library and have designed our app extensively around it, and I hope you see it as a success that you've written a framework that a random engineer cares about enough to write an extremely long-winded comment on a relatively small API decision.

A middle ground that personally I still don't love but at least provides an option and some documentation would be to add an option to the BlocListener constructor that unsubscribes on deactivate and resubscribes on activate. It would need to be added to places like BlocConsumer, MultiBlocListener, etc. and I personally don't love APIs that require you to do something like dontUsePoisonTippedNeedles: true, but it's an option worth mentioning. Worse comes to worst, perhaps you wouldn't mind exposing _subscribe and _unsubscribe onto BlocListener via BlocListenerBase so that we can create our own subclass of BlocListener that defaults to this behavior without needing to fork anything.

[tenhobi]

@mattalbr thanks for you answer. By my logic, if you say that from navigation stack i.e.

• page A
• page B
• page C -- with provider / listener

you will replace C by D to for example

• page A
• page B
• page D -- without that bloc provider/listener

if the listener still works, that is for sure weird and not wanted. I have never had such issue tho, that listeners from old pages work when they are not in the stack.

However if that is really the case, which I am not sure yet, the issue would be with Provider package, since that is what is used for BlocProvider and BlocListener, no?

[mattalbr]

Sorry for the delayed response, was out of the country for a few weeks.

That is correct -- from what we can tell it appears to be platform dependent. We've only been able to reproduce on iOS and the iOS simulator. We have a page that only ever navigates out via pushReplacementNamed, whose listeners were firing (confirmed via breakpoints in debugger on iOS simulator) from one of the pages that it navigates to.

As far as I can tell, flutter gives no promises that dispose will be called immediately when the widget is removed from the tree, just eventually. Maybe it's intended that it does, but it's hard to say since flutter docs tend to be quite vague about the nitty gritty details around execution.

If it would be helpful, I should be able to pull up that commit and repro with any additional debugging info.

Even without the pushReplacement stuff exacerbating this issue, we still would love to be able to unsubscribe on deactivate -- we strongly believe that the current behavior of listening to child routes in bloclistener, at least as the default, is not something we want to be doing, and empirically this keeps turning into bugs for us at intervals just long enough that we all forgot about the behavior. Is there any path you're comfortable with that would allow us to customize this behavior if you're opposed to changing the default for the whole library? Our alternative without library support would be to subclass every Bloc widget that takes a listener, make listenWhen always check if the current route is active first, and add a lint check that our app never uses the original widgets, but it's not our favorite, and we would also love to do our part by making the API more intuitive for future users to come if possible.

[felangel]

Sorry for the delayed response, was out of the country for a few weeks.

That is correct -- from what we can tell it appears to be platform dependent. We've only been able to reproduce on iOS and the iOS simulator. We have a page that only ever navigates out via pushReplacementNamed, whose listeners were firing (confirmed via breakpoints in debugger on iOS simulator) from one of the pages that it navigates to.

As far as I can tell, flutter gives no promises that dispose will be called immediately when the widget is removed from the tree, just eventually. Maybe it's intended that it does, but it's hard to say since flutter docs tend to be quite vague about the nitty gritty details around execution.

If it would be helpful, I should be able to pull up that commit and repro with any additional debugging info.

Even without the pushReplacement stuff exacerbating this issue, we still would love to be able to unsubscribe on deactivate -- we strongly believe that the current behavior of listening to child routes in bloclistener, at least as the default, is not something we want to be doing, and empirically this keeps turning into bugs for us at intervals just long enough that we all forgot about the behavior. Is there any path you're comfortable with that would allow us to customize this behavior if you're opposed to changing the default for the whole library? Our alternative without library support would be to subclass every Bloc widget that takes a listener, make listenWhen always check if the current route is active first, and add a lint check that our app never uses the original widgets, but it's not our favorite, and we would also love to do our part by making the API more intuitive for future users to come if possible.

I’d love to take a closer look if you could provide a link to a minimal reproduction sample with detailed steps for how to reproduce the issue, thanks!

[mattalbr]

I will try in my spare time, but minimal repro sample might be hard for something like this where we're working with optimized code and race conditions. I know we can reliably repro with our production app at a particular commit, but that might not translate to extremely simple repro attempts.

In the meantime, do you have any comments re: the sub-route listening and either allowing customizing this behavior or changing the default?

[felangel]

In the meantime, do you have any comments re: the sub-route listening and either allowing customizing this behavior or changing the default?

If a navigation stack was replaced and listeners are still firing then I would consider that a bug that needs to be fixed. I’d rather not add workarounds to the bloc library. If you’re able to share a reproduction sample I’m happy to take a closer look and work on a fix asap, thanks!

[mattalbr]

I meant the case where the navigation stack is not replaced, but merely pushed onto, as that is a case we also are concerned about

[felangel]

I meant the case where the navigation stack is not replaced, but merely pushed onto, as that is a case we also are concerned about

If you push a new page onto the navigation stack the widgets below are still mounted and will continue to behave as such. This is behaving as expected because you often still want a listener to fire even if it's not visible (e.g. to show a snackbar, push a dialog, notify another component, etc.).

Can you provide some additional context regarding your use case for not wanting listeners to trigger unless they are at the top of the navigation stack? Thanks!