transistorsoft / flutter_background_fetch

Periodic callbacks in the background for both IOS and Android. Includes Android Headless mechanism
MIT License
567 stars 165 forks source link

[BUG] Creating a new FlutterEngine into HeadlessTask.java, messes up with the cache written by HydratedBloc #98

Closed TheWCKD closed 4 years ago

TheWCKD commented 4 years ago

Your Environment

To Reproduce Steps to reproduce the behavior:

  1. Store state of a bloc into cache after I modify a value (let's say setting a value to 30)
  2. The state gets stored into the cache (as 30), I close the app, I run backgroundheadlesstask, succeded, open the app, the value is 30.
  3. Then set the value to 60, close the app, run backgroundheadlesstask succesfully, but when I open the app again, the value is still set to 30, not 60, since the flutterEngine cache get's only created once into the dispatch() method in HeadlessTask.java file.
  4. No matter what value I set after, it will remain 30 after I run backgroundHeadlessTask and reopen the app.

Note the notificationInterval cache value in the screenshot below which is showing the issue described above. Firstly I set it up to thirty, ran headless background task, remained set to thirty, then set it up to sixty, ran headless background task, but the cached value was still thirty, instead of sixty.

image

christocracy commented 4 years ago

Store state of a bloc into cache after I modify a value (let's say setting a value to 30)

I'm not familiar with "state of a bloc into cache". What does the code look like?

TheWCKD commented 4 years ago

Store state of a bloc into cache after I modify a value (let's say setting a value to 30)

I'm not familiar with "state of a bloc into cache". What does the code look like?

Well, the state of a bloc is actually a class with fields which are being converted into a json and that json is being saved to the cache (the app document storage area). Here's a code that will show you what I mean.

class SettingsState extends Equatable {
  final bool pushNotifications;
  final bool countrySpecific;
  final NotificationInterval notificationInterval;

  SettingsState({
    @required this.pushNotifications,
    @required this.notificationInterval,
    @required this.countrySpecific,
  });

  @override
  String toString() {
    return 'SettingsState($pushNotifications, $countrySpecific, $notificationInterval)';
  }

  @override
  List<Object> get props => [
        pushNotifications,
        notificationInterval,
        countrySpecific,
      ];

  Map<String, dynamic> toJson() {
    return {
      'pushNotifications': pushNotifications,
      'countrySpecific': countrySpecific,
      'notificationInterval': notificationInterval.toString(),
    };
  }

  static SettingsState fromJson(Map<String, dynamic> json) {
    return SettingsState(
        pushNotifications: json['pushNotifications'],
        countrySpecific: json['countrySpecific'],
        notificationInterval: jsonToNotificationInterval(json));
  }

This is the part of the HydratedBloc by @felangel which writes data to the cache.

static Future<HydratedBlocStorage> getInstance({
    Directory storageDirectory,
  }) async {
    if (_instance != null) {
      return _instance;
    }

    final directory = storageDirectory ?? await getTemporaryDirectory();
    final file = File('${directory.path}/.hydrated_bloc.json');
    var storage = <String, dynamic>{};

    if (await file.exists()) {
      try {
        storage =
            json.decode(await file.readAsString()) as Map<String, dynamic>;
      } on dynamic catch (_) {
        await file.delete();
      }
    }

    _instance = HydratedBlocStorage._(storage, file);
    return _instance;
  }

  HydratedBlocStorage._(this._storage, this._file);

  @override
  dynamic read(String key) {
    return _storage[key];
  }

  @override
  Future<void> write(String key, dynamic value) async {
    _storage[key] = value;
    await _file.writeAsString(json.encode(_storage));
    return _storage[key] = value;
  }

  @override
  Future<void> delete(String key) async {
    _storage[key] = null;
    return await _file.writeAsString(json.encode(_storage));
  }

  @override
  Future<void> clear() async {
    _storage.clear();
    _instance = null;
    return await _file.exists() ? await _file.delete() : null;
  }

The issue is the one written above, when running a background task, it created a new flutter engine in the dispatch() method in HeadlessTask.java, and after that it will keep only that engine, thus the state will be saved on that engine only once and not get modified whenever a background task is run again.

The main request would be that it would be great if we could configure whether or not a new flutter engine is created for each background task, because that's causing the issue. The problem is we have code writing to storage, background task reading/writing to storage, followed by more read/writing to storage so when you trigger the 2nd background task it no longer reads from storage as is maintaining its own state which is not in sync with the regular application code.

christocracy commented 4 years ago

Could you fork this repo and develop a simple test-case by modifying the /example for me to operate upon? The /example app currently stores a count in SharedPreferences. Could you modify it to persist to this cache mechanism you're using?

The plugin's HeadlessTask creates an engine in a similar fashion to how android_alarm_manager operates.

TheWCKD commented 4 years ago

Could you fork this repo and develop a simple test-case by modifying the /example for me to operate upon? The /example app currently stores a count in SharedPreferences. Could you modify it to persist to this cache mechanism you're using?

The plugin's HeadlessTask creates an engine in a similar fashion to how android_alarm_manager operates.

Yep, will send the repo after I'll reproduce the bug inside it.

TheWCKD commented 4 years ago

Hello again,

Here's the link to the simple project I created to exemplify the issue.

I have tried to include as many comments in order to explain how bloc works if you're not familiar with it and mainly how the simple counter works with storing and retrieving data to and from the cache.

Steps to reproduce:

  1. Get packages from pubspec.yaml
  2. Run the app in release flutter run --release
  3. Notice that I also included some prints to help you out debugging the situation easily.
  4. You can now see that the cacheState is null, because when you first run the app there is no cached state.
  5. Increment the counter to the value 2 let's say, so the CounterState will be saved in the cache having the counter value set to 2.
  6. Close the app by pressing back, or simply kill it.
  7. Run the background task by executingadb shell cmd jobscheduler run -f com.example.counter -359368280 (i hope it's the same id for you too, dunno how it generates)
  8. The background task should also increment the counter by one if you pay attention to the impelmentation of the headlesstask. And indeed, it does, after you open the app again the counter value is 3.
  9. Increment the value again to let's say 5.
  10. Now repeat the process, kill the app, run the headlesstask, and you would expect that the value will be 6 when you open the app right?
  11. Well, this is where the issue comes into place, the value shows 4, because it incremented the previous saved cache, not the actual state.

And I guess that happens when there is a new flutterEngine created into the HeadlessTask.java file.

Hope I was clear enough.

christocracy commented 4 years ago

Now repeat the process, kill the app, run the headlesstask, and you would expect that the value will be 6 when you open the app right?

I do see 6 (Pixel 3a @ 10.0 & Nexus 5 @ 6.0.1) in both debug and release

christocracy commented 4 years ago
$ flutter doctor
Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, v1.12.13+hotfix.8, on Mac OS X 10.15.3 19D76, locale en-CA)

[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.2)
[✓] Xcode - develop for iOS and macOS (Xcode 11.3.1)
[✓] Android Studio (version 3.5)
[✓] VS Code (version 1.41.1)
[✓] Connected device (3 available)
christocracy commented 4 years ago

I can get it to reproduce #10 when I use Back button. It seems to operate correctly when terminated via swipe.

TheWCKD commented 4 years ago

Now repeat the process, kill the app, run the headlesstask, and you would expect that the value will be 6 when you open the app right?

I do see 6 (Pixel 3a @ 10.0 & Nexus 5 @ 6.0.1) in both debug and release

I guess you killed the up directly from memory (in this case I'm noticing that it also shows 6 for me), but what if you just close the app by pressing the back button and repeat the entire process, will it still show 6, or 4? Because on my physical Galaxy S8 it shows 4.

TheWCKD commented 4 years ago

I can get it to reproduce #10 when I use Back button. It seems to operate correctly when terminated via swipe.

Yup, exactly.

TheWCKD commented 4 years ago

I managed to somehow fix the issue by modifing the HeadlessTask.java this way.

image

But when I run the app somehow I get an exception and the app crashes sometimes with this error.

image

Maybe this helps.

christocracy commented 4 years ago

Not bad, but this is definitely required:

sHeadlessTaskRegistered.set(true);
christocracy commented 4 years ago

If I apply all your changes except for removing sHeadlessTaskRegistered.set(true);, it seems to work fine for me.

TheWCKD commented 4 years ago

If I apply all your changes except for removing sHeadlessTaskRegistered.set(true);, it seems to work fine for me.

I have saved these exact changes into HeadlessTask.java and set the pubspeck.yaml package to path: the path of the modified package.

image

Unfortunately, it still doesn't work for me when pressing the back button. It will still show 4 at #10

christocracy commented 4 years ago

You don't need to modify pubspec.yaml. Just open the /android folder as a separate project in Android Studio and operate directly upon background_fetch project.

christocracy commented 4 years ago

TheWCKD commented 4 years ago

@christocracy You mean editing the file directly from the dependencies folder into flutter? I don't think it makes any difference though. Will try it out in 10 minutes and let you know. As I said I forked your project from github, then modified the file, then set the dependecy with its modified path to pubspec.yaml, ran packages get, worked fine but the issue is still present. :pensive:

christocracy commented 4 years ago

It works for me with both terminate and back button.

TheWCKD commented 4 years ago

It works for me with both terminate and back button.

As shown in this video here, I still have the issue unfortunately, do you have other idea on how to fix the issue? I've been trying to fix this issue for the past 3 days. I appreciate all your help. Do you have discord so that we can communicate faster?

christocracy commented 4 years ago

Can you tell me what it is you're using this Bloc thing for? What sort of data is being persisted?

TheWCKD commented 4 years ago

Can you tell me what it is you're using this Bloc thing for? What sort of data is being persisted?

Bloc is a design pattern and also an arhitecture pattern which makes it easier to maintain state on large Flutter projects. I am using it on a big flutter project and also your package for triggering some background tasks and dispatch some notifications. To answer the second question, a quick answer would be that the state of the app is persisted, just like it would persist by using sharedPreferences.

Just like in the simple counter example, the counter is being persisted so that every time you reopen the application, the counter will not be reset to zero, but receive the latest saved state from cache.

TheWCKD commented 4 years ago

Are you sure that after you modified the HeadlessTask.java, you uninstalled the app, ran flutter clean, then flutter run --release and you were not able reproduce the issue anymore? I can't really get it to work even after I modified the file.

christocracy commented 4 years ago

Are you sure that after you...

Yes. Works for both [< back] and terminate.

TheWCKD commented 4 years ago

@christocracy Try the same thing you did, but close the app by always hitting the back button , instead of alternating back with terminate. For me, if I always close the app by hitting back, the bug will still be present.

christocracy commented 4 years ago

instead of alternating

That works fine too.

christocracy commented 4 years ago

Also, I noticed click [<] that the app still appears in recent list. I thought this could be an issue launching from recents vs launch icon but both work fine for me.

TheWCKD commented 4 years ago

@christocracy That's really strange then... Don't really know what may cause this issue. Tried running the app on an emulated Pixel 3, but unfortunately there's no way to run release apps on emulators.

TheWCKD commented 4 years ago

@christocracy Yup, I guess when the back button is pressed then the app won't be killed completely and the flutter engine will still run somehow in the background. Anyways, I have the same flutter stable version as yours, the same modified files, the same project, we're running the same steps in order to reproduce the issue, the only difference is the device we're running the project on. Maybe it's something related to Samsung's way of killing/closing apps.

christocracy commented 4 years ago

Does building for release really matter?

I have a number of other devices to test upon:

christocracy commented 4 years ago

You might try seeing what you can do with BackgroundFetchPlugin onAttachedToEngine

There's a deprecated method binding.getFlutterEngine() you might try experimenting with. If you want to manually provide that FlutterEngine to HeadlessTask, implement a new static method on HeadlessTask (eg: static void setFlutterEngine(FlutterEngine engine))

TheWCKD commented 4 years ago

@christocracy I think I found why it's working for you, there is a slight difference between the steps you're following to replicate the issue and my steps.

The procedure is the following:

  1. Run the app in release mode (yes this step is neccesary, for me if I launch it in debug mode, when I click back, flutter automatically kills the app and opens it really hard afterwards, like a cold boot)
  2. Increment the counter to 2, close the app by hitting back.
  3. Run background task from adb
  4. Open app, it should say 3. NOW IT'S IMPORTANT THAT YOU ALSO INCREMENT AGAIN TO ANOTHER VALUE , let's say 5, then close the app.
  5. Run adb background task.
  6. Now when you open the app it should be saying 6, but it will say 4. That's the issue I'm facing.
christocracy commented 4 years ago

NOW IT'S IMPORTANT THAT YOU ALSO INCREMENT AGAIN TO ANOTHER VALUE

Ok, yes that's it. Now we're on the same page again. Only [<] causes this. Explicit terminate is fine.

TheWCKD commented 4 years ago

@christocracy Will try and see if I can do something with getFlutterEngine method and let you know.

christocracy commented 4 years ago

Maybe there's something here that might help, intercepting the [<] button to move the MainActivity to background, rather than terminating it.

TheWCKD commented 4 years ago

@christocracy Thank you, will certainly take a look at it.

TheWCKD commented 4 years ago

@christocracy Managed to make app go to background when pressing back, will be testing the issue soon and let you know if it still appears. Thanks!

TheWCKD commented 4 years ago

Unfortunately I'm still getting the same issue even after the suggested steps. 😢 Now it works fine, but after I kill the app, the values are getting reset to a previous updated value.

TheWCKD commented 4 years ago

I have finally made it to work by fixing the back button to get app into the background, will do further testing and if everything is ok I'll close this issue.

christocracy commented 4 years ago

Be sure to post the results. Also, can it be done without modifying plugin?

TheWCKD commented 4 years ago

So, the solution seems to work. Here's what I did.

  1. Downloaded this package and followed the steps there on how to make app go into background when pressing back button, instead of it getting killed. (Make sure you update the move_to_background package with the latest pull request from here
  2. I have set up the exact same task for both states of the app (killed via force close = headless) or running, so that when the app is running, the task will run in the configure method callback and when the app is killed the task will run in the registered headlessTask callback.
  3. I have updated the counter repo with the fixed implementation.

Yes, it can be done without modifying background_fetch plugin, I have tested this approach on latest (0.5.5) version and it works as it should. @christocracy you can take a look over the implementation and see that it works. Thanks for the back button suggestion! Feel free to close the issue when you want and thank you again!

christocracy commented 4 years ago

Great.