Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Scope race conditions #316

Closed
subzero911 opened this issue Mar 30, 2023 · 10 comments
Closed

Scope race conditions #316

subzero911 opened this issue Mar 30, 2023 · 10 comments

Comments

@subzero911
Copy link

subzero911 commented Mar 30, 2023

I pop scope on widget dispose and register a new singleton on a new screen open.
But the new singleton gets into the old scope which would got disposed in a next frame. That's because of screens transitions which has not finished, thus old screen's dispose function has called a bit later than new screen's initState.
I couldn't figure out for a long time why my new singleton disappears!

Proposal:

GetIt.pushNewScope() and GetIt.popScope() shouldn't be imperative.
Force user to register singletons inside of pushNewScope callback

var profileScope = GetIt.pushNewScope(init: (getIt) { 
  getIt.registerSingleton(EditProfileController());
}) 

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

User should pop scope only by name or by instance:

  profileScope.pop();
  // or
  GetIt.popScope(profileScope);
  // or
  GetIt.popScope(name: 'profile');

also related issues #313, #303

@escamoteur
Copy link
Collaborator

GetIt.I.getIt.registerSingleton(YetAnotherController());

isn't there one getIt to much in there?

@subzero911
Copy link
Author

isn't there one getIt to much in there?

Oh, that's a typo.
Of course, I meant GetIt.I.registerSingleton(YetAnotherController());

@escamoteur
Copy link
Collaborator

have you checked the get_it_mixin with it's pushScope method?

@subzero911
Copy link
Author

have you checked the get_it_mixin with it's pushScope method?

No, I didn't. Could it solve my problem?

@escamoteur
Copy link
Collaborator

not 100% sure. are you aware, that all the pop functions are async and return a Future?

@subzero911
Copy link
Author

As far as I can see, pushScope method of get_it_mixin is designed to be called in a widget's build method only.
But I want to push scopes inside the router config: flutter/flutter#102408 (comment)

GoRoute(
        path: '/profile',
        builder: (context, state) => const ProfileScreen(),
        onInit: () => GetIt.pushNewScope(init: (getIt) { getIt.registerSingleton(ProfileController());} )
        onDispose: () => GetIt.popScope();
      ), 

Also, get_it_mixin's pushScope is still imperative, and it can't be guaranteed that other singletons won't get into this scope right before the widget is disposed.

@escamoteur
Copy link
Collaborator

yes, you are right. I think the problem is that PopScope is async

@escamoteur
Copy link
Collaborator

OK, sometimes it takes time to fully understand the problem that people have. The obvious solution is #292.
So if you only want a scope to live as long as a route or widget you should only use disposeScope('scopeName') and not mix it with using popScope. Does that make sense? Alternatively, we could think about adding a separate scope list that is not part of the scope stack that has its own createScope, disposeScope` function, but that would make the object lookup slower.

@escamoteur
Copy link
Collaborator

Included in just pushed V7.5.0

@subzero911
Copy link
Author

@escamoteur I checked out the new function.
But this new dropScope() method does not protect me from getting other singletons into this scope, unless added yet another scope above.

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 'scope1' gots disposed.
Your fix does nothing about it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants