jonataslaw / getx

Open screens/snackbars/dialogs/bottomSheets without context, manage states and inject dependencies easily with Get.
MIT License
10.23k stars 1.61k forks source link

Get.offAll() keeps Widgets in memory #157

Closed duck-dev-go closed 4 years ago

duck-dev-go commented 4 years ago

Describe the bug When I have 2 widgets and I navigate to the second widget and the use Get.offAll() it keeps the widgets in memory. Am I using Get.offAll() correctly?

My signup page has the country picker pluging and I noticed that it is not getting removed so I had almost 50mb on country flags after a while.

I get the message that the route is being removed but the widgets aren't disposed. Also the controllers closed methods aren't being called.

I/flutter (27731): [GOING TO ROUTE] /testwidget1
I/flutter (27731): [REMOVING ROUTE] /testwidget2
I/flutter (27731): [REMOVING ROUTE] /testwidget1
V/AudioManager(27731): querySoundEffectsEnabled...
I/flutter (27731): [GOING TO ROUTE] /testwidget2
V/AudioManager(27731): querySoundEffectsEnabled...
I/flutter (27731): [GOING TO ROUTE] /testwidget1
I/flutter (27731): [REMOVING ROUTE] /testwidget2
I/flutter (27731): [REMOVING ROUTE] /testwidget1
V/AudioManager(27731): querySoundEffectsEnabled...
I/flutter (27731): [GOING TO ROUTE] /testwidget2
V/AudioManager(27731): querySoundEffectsEnabled...
I/flutter (27731): [GOING TO ROUTE] /testwidget1
I/flutter (27731): [REMOVING ROUTE] /testwidget2
I/flutter (27731): [REMOVING ROUTE] /testwidget1
V/AudioManager(27731): querySoundEffectsEnabled...
I/flutter (27731): [GOING TO ROUTE] /testwidget2
V/AudioManager(27731): querySoundEffectsEnabled...
I/flutter (27731): [GOING TO ROUTE] /testwidget1
I/flutter (27731): [REMOVING ROUTE] /testwidget2
I/flutter (27731): [REMOVING ROUTE] /testwidget1
I/flutter (27731): [GOING TO ROUTE] /testwidget2

The non global controllers are also still there in my app.

To Reproduce Copy paste this code and click on the button a few times.

void main()  {
  runApp(GetMaterialApp(home: TestWidget1()));
}

class TestWidget1 extends StatelessWidget
{
  @override
  Widget build(BuildContext context) {
    return RaisedButton(
      child: Text('go to widget 2'),
      onPressed: () => Get.to(TestWidget2())
    );
  }

}

class TestWidget2 extends StatelessWidget
{
  @override
  Widget build(BuildContext context) {
    return GetBuilder<TestController>(
      init: TestController(),
          builder: (TestController testController) => RaisedButton(
        child: Text('off all'),
        onPressed: () => Get.offAll(TestWidget1())
      ),
    );
  }
}

class TestController extends GetController
{
  final int testValue = 5;
}

image

image

According to the documentation I am using it correctly right? So this is a really big problem for me the app is consuming massive amounts of memory because of the flags still in memory.

_To go to the next screen and cancel all previous routes (useful in shopping carts, polls, and tests)

Get.offAll(NextScreen());_

Expected behavior I expect that the Get.offAll() goes to the new the widget and removes all the previous pages.

Flutter Version: 1.17

Get Version: get: ^2.7.1

Describe on which device you found the bug: Huawei p30 pro

jonataslaw commented 4 years ago
import 'package:flutter/material.dart';
import 'package:get/get.dart';

void main() {
  runApp(GetMaterialApp(home: TestWidget1()));
}

class TestWidget1 extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return RaisedButton(
        child: Text('go to widget 2'), onPressed: () => Get.to(TestWidget2()));
  }
}

class TestWidget2 extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return GetBuilder<TestController>(
      init: TestController(),
      builder: (TestController testController) => RaisedButton(
          child: Text('off all'), onPressed: () => Get.offAll(TestWidget1())),
    );
  }
}

class TestController extends GetController {
  final int testValue = 5;
  @override
  void onClose() {
    print("onClose testttt");
    super.onClose();
  }
}

log:

Restarted application in 2.981ms.
I/flutter (24649): [GOING TO ROUTE] /
I/flutter (24649): [GOING TO ROUTE] /testwidget2
I/flutter (24649): [GOING TO ROUTE] /testwidget1
I/flutter (24649): [REMOVING ROUTE] /testwidget2
I/flutter (24649): [REMOVING ROUTE] /
I/flutter (24649): onClose testttt
I/flutter (24649): [GET] onClose of TestController called
I/flutter (24649): [GET] TestController deleted from memory
I/flutter (24649): [GOING TO ROUTE] /testwidget2
I/flutter (24649): [GOING TO ROUTE] /testwidget1
I/flutter (24649): [REMOVING ROUTE] /testwidget2
I/flutter (24649): [REMOVING ROUTE] /testwidget1
I/flutter (24649): onClose testttt
I/flutter (24649): [GET] onClose of TestController called
I/flutter (24649): [GET] TestController deleted from memory
I/flutter (24649): [GOING TO ROUTE] /testwidget2
I/flutter (24649): [GOING TO ROUTE] /testwidget1
I/flutter (24649): [REMOVING ROUTE] /testwidget2
I/flutter (24649): [REMOVING ROUTE] /testwidget1
I/flutter (24649): onClose testttt
I/flutter (24649): [GET] onClose of TestController called
I/flutter (24649): [GET] TestController deleted from memory
I/flutter (24649): [GOING TO ROUTE] /testwidget2
I/flutter (24649): [GOING TO ROUTE] /testwidget1
I/flutter (24649): [REMOVING ROUTE] /testwidget2
I/flutter (24649): [REMOVING ROUTE] /testwidget1
I/flutter (24649): onClose testttt
I/flutter (24649): [GET] onClose of TestController called
I/flutter (24649): [GET] TestController deleted from memory
I/flutter (24649): [GOING TO ROUTE] /testwidget2
I/flutter (24649): [GOING TO ROUTE] /testwidget1
I/flutter (24649): [REMOVING ROUTE] /testwidget2
I/flutter (24649): [REMOVING ROUTE] /testwidget1
I/flutter (24649): onClose testttt
I/flutter (24649): [GET] onClose of TestController called
I/flutter (24649): [GET] TestController deleted from memory

onClose will always be called when a route leaves the stack, in any circumstance, so I recommend a Flutter clean in your project (as shown in the print, with its example it works perfectly), because there is currently no possibility of a Controller being in memory after the widget that started it being removed, in addition to the dispose that is called, there are route callbacks throughout the lib that will do this even if the disposition of a widget fails, so there is no chance of that happening.

As for the widget being in memory (the widget, not the controller), this is a Flutter problem and the pushAndRemoveUntil introduced in 1.17, it also happens with MaterialApp using MaterialPageRoute as well. Anyone using named routes does not face this problem, because pushNamedAndRemoveUntil is normal. If you are using version 1.17 and up, they force the initial route to be '/' even without named routes, so I imagine that theoretically using this will solve your problem:

Get.offAllNamed('/');

This is theory, and if it works well, I might switch from the Get.offAll approach to running Get.offAllNamed('/') under the hood.

duck-dev-go commented 4 years ago

It appears to fix the problem. Quite a big bug tho, wonder why they didn't fix it in the latest release that came out today.

jonataslaw commented 4 years ago

It appears to fix the problem. Quite a big bug tho, wonder why they didn't fix it in the latest release that came out today.

They are recreating the Flutter navigator from scratch. This is not the only bug introduced in 1.17. the Get code has many "seams" to try to soften the effect of most of them. But well, if it worked for you, I'll add it as a default for offAll. I already imagined that this would happen just by reading the commits, but well, with this correction, this will no longer be a problem with Get.

duck-dev-go commented 4 years ago

Okay so I have been struggeling with this alot! And I can't seem to get it work.

I have the following routes in my GetMaterialApp

      namedRoutes: {
        '/' : GetRoute(page: TheSplashView(), binding: AuthenticationBinding()),
        '/signUp': GetRoute(page: TheSignUpView()),
        '/home': GetRoute(page: TheHomeView(), binding: AuthenticatedServiceBinding()),
      },

With the following bindings

class AuthenticationBinding extends Bindings {
  @override
  void dependencies() {
    Get.lazyPut<AuthenticationController>(() => FirebaseAuthenticationController(onAuthenticated: (state) {
      Get.offAllNamed('/home');
    }, onUnauthenticated: (state) {
      Get.offAllNamed('/signUp');
    }));
  }
}

class AuthenticatedServiceBinding extends Bindings {
  @override
  void dependencies() {
    final AuthenticatedUserModel user = Get.find<AuthenticationController>().user;
    Get.lazyPut<DatabaseService>(() => FirestoreService(user));
    Get.lazyPut<NotificationService>(() => CloudMessagingService(user,
        onMessage: (NotificationModel notification) =>
            Get.dialog(PlatformAlertDialog(notification.title, notification.body))));
    Get.lazyPut<StorageService>(() => FirebaseStorageService(user));
  }
}

The authentication controller has the firebase onAuthenticated and onUnauthenticated callback. These will load a page once triggered.

Then I have my splash page as the initial route

class TheSplashView extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    Get.find<AuthenticationController>();
    return MediaQuery.removePadding(
      context: context,
      removeTop: true,
      removeLeft: true,
      removeRight: true,
      child: Scaffold(
        resizeToAvoidBottomInset: false,
        body: PlatformCircularProgressIndicator(),
      ),
    );
  }
}

it get's the AuthenticationController so it's onInit get's triggered and it will subscribe to the firebase onAuthChanged event

  @override
  void onInit() {
    print('init auth');
    _listner =
        _authentication.onAuthStateChanged.listen((FirebaseUser firebaseUser) => _onAuthStateChanged(firebaseUser));
  }

And it will call the callbacks based on the event change

  Future<void> _onAuthStateChanged(FirebaseUser firebaseUser) async {
    if (firebaseUser != null) {
      protectedUser = await FirestoreService(AuthenticatedUserModel.fromFirebase(firebaseUser)).getUser();
      protectedState = AuthenticationState.authenticated;
    } else {
      protectedUser = null;
      protectedState = AuthenticationState.unauthenticated;
    }
    update(this);

    if (state == AuthenticationState.authenticated) {
      onAuthenticated?.call(state);
    } else {
      onUnauthenticated?.call(state);
    }
  }

So I open the app log in and then sign up (the sign up page does not disappear from memory as the new route is pushed) And once authenticated I will sign out with firebase signout . The onAuthChanged will trigger and it will load the signUpPage again and it will duplicate it in memory which causes me to have many mb's of country code flags

image

The controllers from the signup page are also still in memory

image

after

I/flutter (28515): [GOING TO ROUTE] /home I/flutter (28515): [REMOVING ROUTE] /signUp

has been called. How is this possible. Could it have anything to do with my SignUpView() being a statefull widget?

jonataslaw commented 4 years ago

I didn't quite understand your issue, but if there is a problem, for sure the problem is in a Get.find() called at random in the build method. This makes it not be linked with any builder, with any route, with anything.

class TheSplashView extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    Get.find<AuthenticationController>(); // Don't ever do that
    return MediaQuery.removePadding(
      context: context,
      removeTop: true,
      removeLeft: true,
      removeRight: true,
      child: Scaffold(
        resizeToAvoidBottomInset: false,
        body: PlatformCircularProgressIndicator(),
      ),
    );
  }
}
jonataslaw commented 4 years ago

Hi, do you have any updates on this issue? 23k is a lot to stay in memory. Is that data in your controller? In the last version I made several security callbacks to prevent controllers from being in memory, but there is no way to prevent widgets from remaining in memory, this is not handled by Get, but by Flutter. So I always recommend using StatelessWidget (which is self-disposable), and always insert everything you use from variables, TextEditingControllers, functions, anything other than widgets in your Controller. The Flutter dispose may fail, but the Get Controllers dispose does not fail, at least I cannot think of a possibility, since in addition to using the Flutter dispose as a base, Get now uses the route observer as security for if any Controller has not been called with a GetBuilder/GetX dispose, the observer makes the device itself when asked to register a route or return from one.

jonataslaw commented 4 years ago

Without further information it will not be possible to proceed with this issue. If you run into problems, don't hesitate to open another one, or reopen it if necessary.

duck-dev-go commented 4 years ago

Hi my laptop broke down, so was not working on this. I just converted the signup page to a stateless widget and upgraded to the newest get version and I noticed that I got an error from Get. Going to look into that right now.

Quick question tho. When are bindings deleted? I noticed my binding being deleted when my stateless page widget is being disposed by the ofAllNamed(). So it's not like my binding put's the controller/instance in a persistent way but it actually binds it to the lifetime of the widget right? Which is nice but just to be sure I would like to know if this is how it works right now.

duck-dev-go commented 4 years ago

I tried removing the future from the splash page entirely and let a controller handle it. So I put() a splash controller in the GetMaterialApp's onInit()

Get.put(SplashController(onSplashLoadDone));

that executes a onDone() and that put()'s the authentication controller

class SplashController extends GetController {
  Future duration = Future<void>.delayed(Duration(seconds: 2));
  Function onDone;

  SplashController(Function onDone);

  @override
  void onInit() {
    print('splash controller init');
    load();
    super.onInit();
  }

  Future<void> load() async {
    print('splash controller loading');
    await duration;
    onDone?.call();
    print('splash controller done loading');
  }
}

and

  void onSplashLoadDone() {
    Get.put<ConnectionController>(
      ConnectionController(
        onConnected: (result) {
          if (Get.find<AuthenticationController>().state == AuthenticationState.authenticated)
            Get.offAllNamed('/home');
          else
            Get.offAllNamed('/signUp');
        },
        onDisconnected: () {
          Get.offAll(TheDisconnectedView());
        },
      ),
    );
    Get.put<AuthenticationController>(
      FirebaseAuthenticationController(onAuthenticated: (state) {
        Get.offAllNamed('/home');
      }, onUnauthenticated: (state) {
        Get.offAllNamed('/signUp');
      }),
    );
  }
}

But it's onInit doesn't get executed. I remember that I read somewhere that you shouldn't put code in a controller's contructor. So how do I call load() in the splash controller which is being put()?

duck-dev-go commented 4 years ago

So I tried to put the authentication controller after a delay which works. But the strange thing is when I execute Get.offAll() it closes the controllers that I put() in the onInit() of my GetMaterial app

class App extends StatelessWidget {
  final String deviceId;
  final PreferencesController preferencesController;

  App(this.deviceId, this.preferencesController);

  @override
  Widget build(BuildContext context) {
    SystemChrome.setEnabledSystemUIOverlays([]);

    return GetMaterialApp(
      onInit: () => initialize(),
      debugShowCheckedModeBanner: false,
      supportedLocales: [Locale('en', 'US'), Locale('nl', 'NL')],
      defaultTransition: Transition.fade,
      transitionDuration: Duration(microseconds: 0),
      localizationsDelegates: [
        LocalizationService.delegate,
        GlobalMaterialLocalizations.delegate,
        GlobalWidgetsLocalizations.delegate,
      ],
      localeResolutionCallback: (locale, supportedLocales) {
        Locale changedTo = supportedLocales.first;
        for (var supportedLocale in supportedLocales) {
          if (locale != null &&
              supportedLocale.languageCode == locale.languageCode &&
              supportedLocale.countryCode == locale.countryCode) changedTo = supportedLocale;
        }

        final DeviceInfoModel deviceInfo = Get.find<DeviceInfoModel>();
        deviceInfo.updateLanguageCode(changedTo.languageCode);

        if (Get.isRegistred<DatabaseService>()) Get.find<DatabaseService>().setDeviceInfo(deviceInfo);

        return changedTo;
      },
      darkTheme: LightTheme.themeData,
      theme: DarkTheme.themeData,
      namedRoutes: {
        '/': GetRoute(page: TheSplashView()),
        '/disconnected': GetRoute(page: TheDisconnectedView()),
        '/signUp': GetRoute(page: TheSignUpView()),
        '/home': GetRoute(page: TheHomeView(), binding: AuthenticatedServiceBinding()),
        '/group/create': GetRoute(page: TheGroupCreateView(), binding: AuthenticatedServiceBinding()),
        '/group/invitations': GetRoute(page: TheGroupInvitationsView(), binding: AuthenticatedServiceBinding()),
        '/group/:groupId': GetRoute(page: TheGroupView(), binding: AuthenticatedServiceBinding()),
        '/group/members/invite': GetRoute(page: TheGroupInviteView(), binding: AuthenticatedServiceBinding()),
        '/settings': GetRoute(page: TheSettingsView(), binding: AuthenticatedServiceBinding()),
        '/account': GetRoute(page: TheAccountView(), binding: AuthenticatedServiceBinding()),
        '/imagepicker': GetRoute(page: TheImagePickerView()),
      },
    );
  }

  void initialize() {
    Get.changeThemeMode(preferencesController.isDarkMode ? ThemeMode.light : ThemeMode.dark);
    Get.put<DeviceInfoModel>(DeviceInfoModel(deviceId, Platform.operatingSystem));
    Get.put<PreferencesController>(preferencesController);
    Get.put(ContactsService());
    Get.put<OverlayController>(OverlayController());
    Future<void>.delayed(Duration(seconds: 2)).then((value) => onSplashLoadDone());
  }

  void onSplashLoadDone() {
    print('on splash done');
    Get.put<ConnectionController>(
      ConnectionController(
        onConnected: (result) {
          if (Get.find<AuthenticationController>().state == AuthenticationState.authenticated)
            Get.offAllNamed('/home');
          else
            Get.offAllNamed('/signUp');
        },
        onDisconnected: () {
          Get.offAll(TheDisconnectedView());
        },
      ),
    );
    Get.put<AuthenticationController>(
      FirebaseAuthenticationController(onAuthenticated: (state) {
        Get.offAllNamed('/home');
      }, onUnauthenticated: (state) {
        Get.offAllNamed('/signUp');
      }),
    );
  }
}

So the excution order is GetMaterialApp(onInit: () => initialize()) > initialize() > onSplashLoadDone() > onAuthenticated() > Get.offAllNamed('/home');

Once Get.offAllNamed('/home'); is executed the AutenticationController and ConnectionController are deleted that just got put() in the onSplashLoadDone()? Those controllers aren't bound to any widget so how is this possible?

/flutter (27459): [GOING TO ROUTE] /
I/flutter (27459): on splash done
I/flutter (27459): [GOING TO ROUTE] /home
I/flutter (27459): [REMOVING ROUTE] /
I/flutter (27459): authentication controller closed
I/flutter (27459): [GET] onClose of ConnectionController called
I/flutter (27459): [GET] ConnectionController deleted from memory
I/flutter (27459): [GET] onClose of AuthenticationController called
I/flutter (27459): [GET] AuthenticationController deleted from memory

What would make sense to me is if put controllers will stay in memory until you remove them. And bound controllers get removed when the widget is disposed.

jonataslaw commented 4 years ago

Get.put(Controller(), permanent: true); will work to you.