Scope race conditions (continued)

[subzero911]

@escamoteur I checked out this new dropScope() method in v7.5.0. But it does not protect me in any way from getting other singletons into this scope, unless added yet another scope before them.

GoRoute(
  path: '/profile',
  builder: (context, state) => const ProfileScreen(),
  onInit: () {
    getIt.pushNewScope(scopeName: 'scope1');
    getIt.registerSingleton(Foo());
  }
  onDispose: () {
   getIt.dropScope('scope1')
  }),
GoRoute(
  path: '/details',
  builder: (context, state) => const DetailsScreen(),
  onInit: () => getIt.registerSingleton(Bar());
}

On navigating from /profile screen to the /details screen: Expected behaviour: Bar() is registered in the base scope. Actual behaviour: Bar() is registered in the 'scope1', then screen transitioning ends and 'scope1' gots disposed. That fix does nothing about it.

Proposal:

Add protected flag to the pushNewScope, so I can register singletons in the init callback only.

GetIt.pushNewScope(
  protected: true,
  init: (getIt) { 
    getIt.registerSingleton(EditProfileController());
}) 

GetIt.I.registerSingleton(YetAnotherController()); // does not get into the scope above

[escamoteur]

Hmm, interesting. that problem is slightly different than the other race conditions that dropScope solves. The protected flag is an interesting approach. Maybe I name is closedScope or something like that. @esDotDev curious what you think about this.

[esDotDev]

Not sure this is an issue that GetIt needs to solve. The problem is go-router shows a new route before calling dispose, so the OPs solution is not a good choice. GetIt is working as expected, if you call register before calling popScope, of course it ends up in the current scope.

Either GR should have a onTransitionOut callback that fires when it is about to transition out, or come up with some other method to pop scopes that does not rely on GR callbacks.

With that said the isClosed approach would work, but it just seems a bit odd to me... not really sure why. I guess there is nothing inherently wrong with saying "this scope is final", as long as you think it doesn't create a bunch of complexity in the codebase.

[escamoteur]

I just mad a small fix to the get_it_mixin's pushscope, that ensures that the pushed scope has a unique name and it uses that to drop the scope when the widget is destroyed. @subzero911 could you please check if that would solve your problem? using pushScope of the get_it_mix_in in the build function insteat of pushing in the go_router callback?

[escamoteur]

@esDotDev I think an isClosed would be possible without adding too much complexity, but first I'd like to know if the above proposal solves that problem.

[subzero911]

using pushScope of the get_it_mix_in in the build function insteat of pushing in the go_router callback?

but calling pushScope in build() is not what I wanted. I want to have all my registers/unregisters right in the GoRouter setup, so I can see right away which routes use which dependencies.

Either GR should have a onTransitionOut callback that fires when it is about to transition out, or come up with some other method to pop scopes that does not rely on GR callbacks.

I'll create an issue in a GR repo when I find the time, but it's hard to predict whether they'll fix that, and when (their development is veeery slow). It would be nice to have a possibility to solve it on the get_it side.

[escamoteur]

@subzero911 I just added an isFinal parameter to the pushNewScope function on github. Would you be willing to write tests for it and add an explanation to the readme?

[escamoteur]

oh, I was a bit too quick, I forgot to run the the existing test, my bad

[escamoteur]

ok, now all current tests run. @subzero911 are you adding tests and docs?

[subzero911]

Yup, I don't mind helping.

[escamoteur]

Cool, waiting for your PR,

[subzero911]

I see that you already added a good description.

[escamoteur]

yep but not in the readme and if you want to improve the docs, always welcome

[subzero911]

Working on tests. How to check if the object is in a certain scope?

[escamoteur]

I think you can only test it indirekt by first pushing a closed scope, then registering and popping the scope and testing if it is still there

[subzero911]

Also I found a little bug. You haven't added isFinal to declaration, but to implementation only; it's impossible to use it.

[escamoteur]

Actually it turns out that this new feature allows a nice precausion that possible might already solve your problem even if you don't declare your scope as final. I now make any scope that is being popped/dropped final before calling the async functions

[subzero911]

Actually it turns out that this new feature allows a nice precausion that possible might already solve your problem even if you don't declare your scope as final.

No, it won't solve, if I registered a new singleton right before popScope() was called. I still have to set isFinal to true manually.

I added yet another PR https://github.com/fluttercommunity/get_it/pull/330

[esDotDev]

I do wonder if a warning should be printed out if you attempt to register an object with a final scope.

In this case it would be expected, as the OP wants to essentially call methods in the wrong order, but still have things work. (Should be push > registerFoo > pop > registerBar, but OP wants to do push > registerFoo > registerBar > pop)

But in other cases, where this is not desired, it might be nice to at least indicate to the user that they might be doing something unexpected? I would guess that the expected/intuitive use case for isFinal would be specifically that: I am expecting this scope to be final, please tell me if any registrations are attempted on it (and probably prevent them).

Just a thought.

I also wonder why OP doesn't just register the details singleton when the app starts up, as it is never disposed of. Seems like a lazy singleton would be perfect here, and sidestep this whole issue of trying to register a singleton in the base scope, while another scope exists during transition. Then you end up with the easier to follow: registerBar > push > registerFoo > pop

[esDotDev]

Also, just thinking out loud, but if we could access the list of scopes, we could probably also solve the problem that way, a bit more directly.

getIt.registerSingleton(Bar(), scope: getIt.scopes.first);

or maybe

getIt.scopes.first.registerSingleton(Bar());

[subzero911]

I also wonder why OP doesn't just register the details singleton when the app starts up, as it is never disposed of

Why have you decided it's never disposed? I will dispose it in the onDispose callback as well. (i didn't put it into example to keep the piece of code clear). I don't want to register it on startup, because it's a local controller which should be disposed when his screen's unmounted.

[escamoteur]

You mean if the top level scope is final and someone tries to register something instead of silently register it in the next open scope? Am 26. Mai 2023, 22:09 +0200 schrieb Shawn @.***>:

I do wonder if a warning should be printed out if you attempt to register an object with a final scope. In this case it would be expected, as the OP wants to essentially calls methods in the wrong order, but still have things work. But in other cases, where this is not desired, it might be nice to at least indicate to the user that they might be doing something unexpected? Just a thought. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[escamoteur]

What would be easy is to add an optional scopename when registering. That way you could make sure it gets in the scope you want. Might not often be needed but maybe in some edge cases it could be helpful Am 26. Mai 2023, 22:14 +0200 schrieb Shawn @.***>:

Also, just thinking out loud, but if we could access the list of scopes, we could probably also solve the problem that way, a bit more directly. GoRoute( path: '/details', builder: (context, state) => const DetailsScreen(), onInit: () => getIt.registerSingleton(Bar(), scope: getIt.scopes.first); } — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[esDotDev]

Why have you decided it's never disposed?

Just based on the example.

If it's just local controller, why use scopes at all? Couldn't you just register the singleton in initState, and unregister it in dispose? This would be more robust anyways, as the view could be shown using an unnamed route and still continue to work. And you don't get into any hacky stuff with a view transitioning in while another transitioning out, and tieing your dispose() calls to those out-of-order method calls.

If the boilerplate bothers you, I think you could probably make a generic mixin that handles this easily enough too... psuedocode:

class LocalControllerMixin<T> on State {

  T get controller; // each view must implement this

  void initState(){
    getIt.registerSingleton<T>(getController);
  }

  void dispose(){
    getIt.unregister<T>();
  }

}

Then a view would just need something like:

MyViewState extends State with LocalControllerMixin<Bar> {
   Bar get controller => Bar();
}

You mean if the top level scope is final and someone tries to register something instead of silently register it in the next open scope?

Ya exactly, this sort of magic behavior of registering in the next available scope seems like it could be confusing / problematic, hard to debug race conditions etc.

[subzero911]

Say, in 1st route it's multiple controllers, and I decided to put them into scope. In 2nd route it's 1 controller, and I'll just register and dispose it, without scopes. For such an edge case I need isFinal for the 1st scope.

[esDotDev]

Sure, but only because you want to register a new singleton before the old one is popped, which is because your callback is firing later than it should be, which is an implementation detail of GR.

Maybe it's fine? Just trying to play devils advocate here, really this is not GetIt's problem, and I'd just be careful about adding a specific feature for this one issue, unless the feature actually has value on its own.

I think having isFinal but showing a warning when scopes are pushed to it might be a nice middle ground. Then the feature exists primarily as you would expect it to, which is to indicate that no new singletons should be registered while this scope exists, but then also opening the door to this somewhat hacky approach where you can register something with a final scope, and rely on it implicitly being assigned to the latest non-final scope.

[escamoteur]

I'd actually leaning to make all scopes final by default if an initFunction is given and throwing an exception if you try to register something outside the init scope unless you enable a static flag 'automaticRegisterInNextopenScope because I agree if a scope was declared final or closed (still not sure what's the better name)  a registration outside of init it probably an error Am 26. Mai 2023, 23:19 +0200 schrieb Shawn @.***>:

Sure, but only because you want to register a new singleton before the old one is popped, which is because your callback is firing later than it should be, which is an implementation detail of GR. Maybe it's fine? Just trying to play devils advocate here, really this is not GetIt's problem, and I'd just be careful about adding a specific feature for this one issue, unless the feature actually has value on its own. I think having isFinal but showing a warning when scopes are pushed to it might be a nice middle ground. Then the feature exists primarily as you would expect it to, which is to indicate that no new singletons should be registered while it exists, but then also opening the door to this somewhat hacky approach where you can register something, and rely on it implicitly being assigned to the latest non-final scope. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[esDotDev]

It does strike me that the optional scope name is the way to go, as it is the most clear and does not do any implicit behavior, and it solves for all cases where order-of-operations is causing some issues.

But I'm not sure how we would make that work in the case where you want to use the first non-named scope... as passing scope: null could mean use the top-most scope, or it could mean use the first non-named scope.

But maybe we don't? Maybe it's just a requirement to use named scopes for this sort of use case.

Proposal

The main downside to forcing scopes here is the extra boilerplate. What if calling registerSingleton(..., scope: "a") would automatically call pushScope if one doesn't exist for that name?

Then the example above could be written as:

GoRoute(
  path: '/profile',
  builder: (context, state) => const ProfileScreen(),
  onInit: () => getIt.registerSingleton(Foo(), scope: 'profile');
  onDispose: () => getIt.dropScope('profile'),
),
GoRoute(
  path: '/details',
  builder: (context, state) => const DetailsScreen(),
  onInit: () => getIt.registerSingleton(Bar(), scope: 'details');
  onDispose: () => getIt.dropScope('details'),
}

Now there is no concern about order of operations, and no magic behavior. Optionally a developer could manually call pushScope if they need more control (async push), or want their code to be fully explicit / more readable.

[escamoteur]

I'm with you that the implicit behaviour isn't a good solution and that the optional name is a good idea. Actually the first named scope has a name 'baseScope' (maybe we could rename it to get_it) Or null means the baseScope and scopeName 'GetIt' means the most recent Scope and that is the default value for the scope parameter. I like the idea of automatically pushing a new scope when registering something. Only drawback would be that you can't provide a scope wide dispose function but would require to pass a disposal function for the object that is registered. That's actuallly my main concern. Although it may make the mechanic more popular to implement the disposal interface in the objects you register inside GetIt, that's probably unknown to most GetIt users despite the fact that it's the most elgegant way.

So either we rethink how disposal is implemented in future get_it or I'm not really pro automatic creation of scopes when registering.

Definitely removing the automatic registration in the base scope if the top scope is final.

Questions:

• final or closed, what communicates better what is meant?
• should scopes that have an init function be closed automatically?

• How do we signal that a registration should go into the baseScope/ top scope if no name is given?

  if someone tries to register in a closed scope we throw an exception/assertion

[subzero911]

I'd actually leaning to make all scopes final by default if an initFunction is given and throwing an exception if you try to register something outside the init scope

That's basically what I proposed in the first post https://github.com/fluttercommunity/get_it/issues/316#issue-1648168911

[subzero911]

final or closed, what communicates better what is meant?

final or sealed

Actually the first named scope has a name 'baseScope' (maybe we could rename it to get_it)

Please don't do, baseScope sounds well

[esDotDev]

Ah I didn't realize these names already existed. I think we'd want to make them const values so that users of the lib can reference them with compile time safety (ish), and then the named scopes feature seems pretty solid and should fully address this use case.

[escamoteur]

Would you make it static const members or just independent constants? Am 29. Mai 2023, 17:14 +0200 schrieb Shawn @.***>:

Ah I didn't realize these names already existed. I think we'd want to make them const values so that users of the lib can reference them, and then the named scopes feature seems pretty solid and should fully address this use case. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[esDotDev]

Personally I like static consts, because naming imports is a pain, and without named imports (or a class name) it hurts readability imo.

But I think this is rather subjective.

[esDotDev]

We could also potentially have a firstNonFinal name, which would allow the user to explicitly register an instance in the first non-final scope, while not providing that name would cause an error if the current scope is final.

Might be a good idea to prefix these as to avoid any potential conflicsts with user strings:

class GetItScopes {
  static const string base = 'getIt-scope-base`;
  static const string firstNonFinal = 'getIt-scope-firstNonFinal`;
}

[escamoteur]

Yeah, that could be a nice option. I'm just pondering what we should do if a new registration occurs while the popScope isn't finished and was not awaited. If we don't automatically register in the next nonfinal scope. Would it be then better to introduce another internal scopestate 'disposing' and throw an assertion if a registration occurs in that phase a la 'Please await 'pop/dropScope' before registering another object?

[jostney]

Nice to see this topic, trying to implement same logic.

@subzero911 Just wondering which version of go_router is this ? I never see onInit and onDispose methods on ^7.1.1 ?

[subzero911]

Nice to see this topic, trying to implement same logic.

@subzero911 Just wondering which version of go_router is this ? I never see onInit and onDispose methods on ^7.1.1 ?

It's my own wrapper. GoRoute doesn't have such callbacks.

[jostney]

I'm using get_it and go_router.

Would you mind if you shared it ? How did you make the GoRoute lifecycle aware ?

[subzero911]

import 'package:flutter/widgets.dart';

class GoRouteWrapper extends StatefulWidget {
  const GoRouteWrapper({
    required this.child,
    this.onInit,
    this.onChangeDependencies,
    this.onAfterLayout,
    this.onDispose,
    super.key,
  });

  final Widget child;
  final void Function()? onInit;
  final void Function(BuildContext context)? onChangeDependencies;
  final void Function(BuildContext context)? onAfterLayout;
  final void Function(BuildContext context)? onDispose;

  @override
  State<GoRouteWrapper> createState() => _GoRouteWrapperState();
}

class _GoRouteWrapperState extends State<GoRouteWrapper> {
  @override
  void initState() {
    super.initState();
    widget.onInit?.call();
    if (widget.onAfterLayout == null) return;
    WidgetsBinding.instance.addPostFrameCallback((_) {
      widget.onAfterLayout?.call(context);
    });
  }

  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    widget.onChangeDependencies?.call(context);
  }

  @override
  void dispose() {
    widget.onDispose?.call(context);
    super.dispose();
  }

  @override
  Widget build(BuildContext context) => widget.child;
}

Usage:

       GoRoute(
          name: RouteNames.newTask,
          path: 'new-task',
          builder: (context, state) => GoRouteWrapper(
            onInit: () => GetIt.I.registerSingleton<TaskCreationController>(TaskCreationController()),
            onDispose: (context) => GetIt.I.unregister<TaskCreationController>(),
            child: NewTaskScreen(editableTask: state.extra as Task?),
          ),
        ),

[escamoteur]

@esDotDev what are your thoughts on this here https://github.com/fluttercommunity/get_it/issues/326#issuecomment-1567932439

[esDotDev]

Yeah, that could be a nice option. I'm just pondering what we should do if a new registration occurs while the popScope isn't finished and was not awaited. If we don't automatically register in the next nonfinal scope. Would it be then better to introduce another internal scopestate 'disposing' and throw an assertion if a registration occurs in that phase a la 'Please await 'pop/dropScope' before registering another object?

I guess I would expect that if a scope if in the process of disposing we can consider it disposed, and act as if it is gone already. So no need to throw an assertion, just use the next scope in the stack and go from there?

[escamoteur]

But what if the next one is final? Am 1. Juni 2023, 17:01 +0200 schrieb Shawn @.***>:

Yeah, that could be a nice option. I'm just pondering what we should do if a new registration occurs while the popScope isn't finished and was not awaited. If we don't automatically register in the next nonfinal scope. Would it be then better to introduce another internal scopestate 'disposing' and throw an assertion if a registration occurs in that phase a la 'Please await 'pop/dropScope' before registering another object? I guess I would expect that if a scope if in the process of disposing we can consider it disposed, and act as if it is gone already. So no need to an assertion, just use the next scope in the stack and go from there? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[esDotDev]

Then it would throw an error, and they need to specify which non-final scope they are targeting?

I would defer to you here though, these are just my initial thoughts on things, and you're much more experienced with various use cases. I only ever use scopes for testing, and don't really use the dispose functionality much, so not sure how much my feedback is valid.

[escamoteur]

No worries, I value your opinion very much. It really helps to have a sparing partner in this decisions

BTW can you live with the watch_it naming now? Am 1. Juni 2023, 17:33 +0200 schrieb Shawn @.***>:

Then it would throw an error, and they need to specific which non-final scope they are targeting? I would defer to you here though, these are just my initial thoughts on things, and you're much more experienced with various use cases. I only ever use scopes for testing, and don't really use the dispose functionality much, so not sure how much my feedback is valid. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[jostney]

I'm keeping continue on this thread because i'm having same problem(race condition). @subzero911 I just implemented GoRouteWrapper to invoke init and dispose methods while using go_router. It's working correctly... but have a problem, if i open the pages so fast then the things go wrong.

For example when i navigate to PreferencesScreen page so fast like this PreferencesScreen -> LanguagesScreen -> PreferencesScreen, then the second onInit called before first unregistering of PreferencesViewModel.

As a result it gives me error: PreferencesViewModel is already registered

    GoRoute(
      path: RoutePaths.preferences,
      parentNavigatorKey: parentNavigatorKey,
      builder: (context, state) => GoRouteWrapper(
        onInit: () => GetIt.I.registerSingleton<PreferencesViewModel>(PreferencesViewModel()..init()),
        onDispose: (context) => GetIt.I.unregister<PreferencesViewModel>(disposingFunction: (vm) => vm.onDispose()),
        child: PreferencesScreen(),
      ),
    ),
    GoRoute(
      path: RoutePaths.languages,
      parentNavigatorKey: parentNavigatorKey,
      builder: (context, state) => GoRouteWrapper(
        onInit: () => GetIt.I.registerSingleton<LanguagesViewModel>(LanguagesViewModel()..init()),
        onDispose: (context) => GetIt.I.unregister<LanguagesViewModel>(disposingFunction: (vm) => vm.onDispose()),
        child: LanguagesScreen(),
      ),
    ),

[escamoteur]

if your disposing function returns a future the unregister call has to be awaited to not get into trouble

[escamoteur]

if you need an object only for one page I recommend using the get_it_mixin (soon to be replaced by the watch_it package) and use the pushScope function

[jostney]

In my scenario, I have multiple screens, each with its own corresponding view model. For example, I have a screen called "PreferencesScreen" and its associated view model called "PreferencesViewModel". Similarly, I have a screen called "LanguagesScreen" and its associated view model called "LanguagesViewModel".

The view models are created and destroyed when their respective screens are opened and closed. In other words, when I navigate from one screen to another, the view model for the current screen is created, and when I navigate away from that screen, the view model is destroyed.

Here's an example sequence of screen navigations:

PreferencesScreen -> LanguagesScreen -> PreferencesScreen -> LanguagesScreen -> PreferencesScreen Imagine that I perform the above navigations in approximately 1 second. In this case, the "onInit" and "onDispose" methods of the instances of PreferencesScreen (and LanguagesScreen as well) will be called in different orders, depending on the navigation sequence and timing.

[jostney]

In addition to the previous information, I'd like to mention a class called "GoRouteWrapper." This class is a StatefulWidget that acts as a wrapper for my screens. It takes the screen as a child and allows me to control the lifecycle of the screen, effectively connecting the screen and its view model.

It's important to note that the "onDispose" methods of PreferencesViewModel and LanguagesViewModel, which are associated with the PreferencesScreen and LanguagesScreen, respectively, are empty. This means that the navigation process does not wait for their completion before proceeding to the next screen. In other words, the navigation moves forward without waiting for any cleanup or finalization tasks in these view models to finish.

[escamoteur]

why don't you just register your viewmodel in the initState and remove it in the dispose function of the widget instead of doing it in the routing logic? Have you tried to use names scopes? Not sure what your real problem is