[android_alarm_manager] migrate to the V2 Android embedding #2193

bkonyi · 2019-10-15T16:33:01Z

Description

Minimum work to migrate to the Android v2 embedding.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

bkonyi · 2019-10-15T16:35:30Z

@matthew-carroll is there anything else here that I missed that's specific to background execution? Also, do we still think onViewDestroyed is necessary or is there a new equivalent for FlutterEngine?

collinjackson

Right now this plugin has no unit tests or integration tests. I think that this android_alarm_manager should be straightforward to integration test using the e2e plugin. Would you be open to adding that?

I wrote some example tests that I think should work as a starting point. Feel free to copy these into the PR.

https://github.com/collinjackson/plugins/tree/android_alarm_manager_test/packages/android_alarm_manager/example/test_driver

Note: these tests do not pass for me when I run them under flutter drive (the alarm manager doesn't seem to trigger), and I'm not sure why. Maybe the alarm manager only works when the app is in the background?

matthew-carroll · 2019-10-15T22:01:30Z

.../android/src/main/java/io/flutter/plugins/androidalarmmanager/AndroidAlarmManagerPlugin.java

+    final AndroidAlarmManagerPlugin plugin = new AndroidAlarmManagerPlugin();
+    // Listen for FlutterView destruction so that this plugin can move itself
+    // to background mode.
+    registrar.addViewDestroyListener(plugin);


@bkonyi do you know what the analogous behavior of this plugin is when using the new embedding? I assume that what this plugin is doing when the view is destroyed is important, right? If so, we won't be able to act on view destruction, but plugins will now have the opportunity to take action when the activity disappears, which is probably what this view destruction really signified in the first place...

Actually, looking at this code now I'm uncertain as to what it even accomplishes. It appears that on view destruction we try and store the view in AlarmService... but we also create a view in AlarmService.startBackgroundIsolate. I think we're safe to remove this... I'll try and see if it causes any issues.

mklim

Looking good. It just needs tests like @collinjackson pointed out.

mklim · 2019-10-15T20:22:30Z

packages/android_alarm_manager/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 0.4.5
+* Add support for Flutter Android embedding V2
+* Called WidgetsFlutterBinding.ensureInitialized() in


I don't think this hurts, but in general we've decided that this is the responsibility of the plugin user, not author.

Fair point. I can remove this from the initialize method, but is there any reason why we're leaving this up to the plugin user?

.../android/src/main/java/io/flutter/plugins/androidalarmmanager/AndroidAlarmManagerPlugin.java

mklim · 2019-10-16T01:09:04Z

.../android/src/main/java/io/flutter/plugins/androidalarmmanager/AndroidAlarmManagerPlugin.java

+    final AndroidAlarmManagerPlugin plugin = new AndroidAlarmManagerPlugin();
+    // Listen for FlutterView destruction so that this plugin can move itself
+    // to background mode.
+    registrar.addViewDestroyListener(plugin);


bkonyi · 2019-10-17T16:11:15Z

Right now this plugin has no unit tests or integration tests. I think that this android_alarm_manager should be straightforward to integration test using the e2e plugin. Would you be open to adding that?

I wrote some example tests that I think should work as a starting point. Feel free to copy these into the PR.

https://github.com/collinjackson/plugins/tree/android_alarm_manager_test/packages/android_alarm_manager/example/test_driver

I'm definitely open to adding tests if we can show they're not going to be extremely flaky. The AlarmManager alarms aren't as predictable as I'd like, but I'll see what I can do.

Note: these tests do not pass for me when I run them under flutter drive (the alarm manager doesn't seem to trigger), and I'm not sure why. Maybe the alarm manager only works when the app is in the background?

These tests run on an actual device, correct? There might be an issue with the phone being in doze mode which results in background events being delayed until the device is in a more "active" state to conserve battery life.

bkonyi · 2019-10-17T18:57:37Z

@collinjackson it looks like driver tests don't work correctly when additional isolates are spawned (see flutter/flutter#24703). It looks like isolates are being paused on start and the driver only resumes the main isolate, regardless of how the test is launched (both flutter drive and the Gradle e2e) so the background isolate for the alarm manager never actually runs.

matthew-carroll · 2019-10-17T19:20:42Z

...alarm_manager/android/src/main/java/io/flutter/plugins/androidalarmmanager/AlarmService.java

@@ -152,15 +152,6 @@ public static void setCallbackDispatcher(Context context, long callbackHandle) {
    prefs.edit().putLong(CALLBACK_HANDLE_KEY, callbackHandle).apply();
  }

-  public static boolean setBackgroundFlutterView(FlutterNativeView view) {


Notice that this isn't really setting a View, it's setting a FlutterNativeView, which is now essentially represented by FlutterEngine. Are you sure this method can be deleted? Where is this behavior handled now?

I think this is an artifact from the original implementation of this plugin when we were running all callbacks on the main isolate, which meant we had to be able to handle the case when the application was moved from foreground to background. This plugin now instead starts a background isolate immediately when AndroidAlarmManager.initialize is invoked or when the AlarmService is started via an Intent, and this isolate is kept alive until the service shuts down.

.../android/src/main/java/io/flutter/plugins/androidalarmmanager/AndroidAlarmManagerPlugin.java

collinjackson · 2019-10-17T23:32:58Z

@collinjackson it looks like driver tests don't work correctly when additional isolates are spawned (see flutter/flutter#24703). It looks like isolates are being paused on start and the driver only resumes the main isolate, regardless of how the test is launched (both flutter drive and the Gradle e2e) so the background isolate for the alarm manager never actually runs.

The Gradle e2e method doesn't use Flutter driver, so I'm surprised it doesn't work. I wonder if @vishna's workaround in flutter/flutter#24703 (comment) would be helpful here.

bkonyi · 2019-10-18T17:49:33Z

@collinjackson it looks like driver tests don't work correctly when additional isolates are spawned (see flutter/flutter#24703). It looks like isolates are being paused on start and the driver only resumes the main isolate, regardless of how the test is launched (both flutter drive and the Gradle e2e) so the background isolate for the alarm manager never actually runs.

The Gradle e2e method doesn't use Flutter driver, so I'm surprised it doesn't work. I wonder if @vishna's workaround in flutter/flutter#24703 (comment) would be helpful here.

Disregard, for some reason the background callback dispatcher wasn't calling WidgetsFlutterBinding.ensureInitialized so the isolate was started but was unable to communicate that back to the plugin. Also, that workaround didn't seem to work before, but I'll give it another shot to see if I can get this working with flutter drive as well.

However, I am still running into some issues with plugins seemingly not being recognized when running tests. It looks like the alarms are firing correctly but we're failing to write the count to the temporary file. We were swallowing the exception before, but I'm seeing this failure:

MissingPluginException(No implementation found for method getTemporaryDirectory on channel plugins.flutter.io/path_provider)

I'm also seeing this warning in the logs: Warning: E2E test plugin was not detected.

I've tried cleaning the project and re-running and haven't had any luck. The GeneratedPluginRegistrant looks like it's setup correctly, so I'm uncertain as to what's going on here...

...le/android/app/src/main/java/io/flutter/plugins/androidalarmmanagerexample/MainActivity.java

xster · 2019-11-05T00:16:37Z

@bkonyi, @collinjackson, anything blocking this PR currently?

bkonyi · 2019-11-05T00:19:31Z

@bkonyi, @collinjackson, anything blocking this PR currently?

Ah sorry for the delay. I think the migration is mostly done, but I'll need to upload my most recent changes. However, as discussed with @matthew-carroll and @mklim we're going to delay publishing this until the next stable is released to handle registering plugins with the background isolate automatically.

matthew-carroll · 2019-11-05T23:47:41Z

...android/src/main/java/io/flutter/plugins/androidalarmmanager/BackgroundExecutionContext.java

+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+public class BackgroundExecutionContext implements MethodCallHandler {


You may want to avoid the term Context on Android when not referring to an Android Context. It might be confusing for Android devs. Another name that might be more descriptive for this object could be FlutterBackgroundExecutor

Also, can you add an appropriate class javadoc?

...alarm_manager/android/src/main/java/io/flutter/plugins/androidalarmmanager/AlarmService.java

matthew-carroll · 2019-11-05T23:49:18Z

...alarm_manager/android/src/main/java/io/flutter/plugins/androidalarmmanager/AlarmService.java

-      sBackgroundFlutterView.runFromBundle(args);
-      sPluginRegistrantCallback.registerWith(sBackgroundFlutterView.getPluginRegistry());
-    }
+    assert (backgroundExecutionContext == null);


assertions won't have any effect in release mode. In fact, I'm not even sure Android supports assert() at all. What should the behavior be if this happens in a real app?

matthew-carroll · 2019-11-05T23:49:56Z

...alarm_manager/android/src/main/java/io/flutter/plugins/androidalarmmanager/AlarmService.java

  }

  /**
-   * Called once the Dart isolate ({@code sBackgroundFlutterView}) has finished initializing.
+   * Called once the Dart isolate ({@code backgroundFlutterView}) has finished initializing.


Is backgroundFlutterView still the correct concept? There shouldn't be any "views" associated with background execution, I don't think.

No, this was an oversight. Thought I had caught all the instances of backgroundFlutterView in the code base... will remove.

matthew-carroll · 2019-11-05T23:50:39Z

...alarm_manager/android/src/main/java/io/flutter/plugins/androidalarmmanager/AlarmService.java

   *
   * <p>Invoked by {@link AndroidAlarmManagerPlugin} when it receives the {@code
   * AlarmService.initialized} message. Processes all alarm events that came in while the isolate
   * was starting.
   */
-  // TODO(mattcarroll): consider making this method package private
-  public static void onInitialized() {
+  static void onInitialized() {


I think we have a style pattern of placing some kind of commented identifier before package private members, e.g., /* package */. Would you mind adding one here?

Ah I wasn't aware. I'll go ahead and do that.

matthew-carroll · 2019-11-05T23:55:48Z

.../android/src/main/java/io/flutter/plugins/androidalarmmanager/AndroidAlarmManagerPlugin.java

+  private Object initializationLock = new Object();
+  private MethodChannel alarmManagerPluginChannel;
+
+  private static AndroidAlarmManagerPlugin singleton;


FYI, in the embedding I always place static members above instance members, and other places where I've created singletons I do so by calling it instance. Up to you if you'd like to replicate that here.

...android/src/main/java/io/flutter/plugins/androidalarmmanager/BackgroundExecutionContext.java

matthew-carroll · 2019-11-05T23:58:21Z

...android/src/main/java/io/flutter/plugins/androidalarmmanager/BackgroundExecutionContext.java

+
+  private AtomicBoolean isIsolateRunning = new AtomicBoolean(false);
+
+  private static PluginRegistrantCallback pluginRegistrantCallback;


If you'd like to match the embedding, all static members are placed above instance members.

matthew-carroll · 2019-11-05T23:59:50Z

...android/src/main/java/io/flutter/plugins/androidalarmmanager/BackgroundExecutionContext.java

+
+  private static PluginRegistrantCallback pluginRegistrantCallback;
+
+  public static void setPluginRegistrant(PluginRegistrantCallback callback) {


Can you add a javadoc for this method? It's probably a good idea to point out when this is expected to be called, and also what kind of consequences might occur by failing to set this at the appropriate time.

matthew-carroll · 2019-11-06T00:01:13Z

...android/src/main/java/io/flutter/plugins/androidalarmmanager/BackgroundExecutionContext.java

+      return;
+    }
+
+    FlutterMain.ensureInitializationComplete(context, null);


On master this call should no longer be needed.

Is this still required for apps using the V1 embedding?

Yeah I think so. In short, if a Flutter experience is based on a FlutterEngine then that call is not needed. But if it's based on FlutterNativeView then it is.

matthew-carroll

LGTM with possibly one remaining question that I left.

This plugin is relatively complicated compared to others, and delves into areas of Android that are very difficult to reason about, so I'll just add that beyond an LGTM, I encourage us to exercise this plugin wherever, whenever, and however we can. If any bugs exist, we're likely to only realize it with a stacktrace.

bkonyi · 2019-11-14T16:55:09Z

LGTM with possibly one remaining question that I left.

This plugin is relatively complicated compared to others, and delves into areas of Android that are very difficult to reason about, so I'll just add that beyond an LGTM, I encourage us to exercise this plugin wherever, whenever, and however we can. If any bugs exist, we're likely to only realize it with a stacktrace.

FYI, I've got changes addressing your earlier comments that are a WIP but I won't get to them until I'm physically in the office next week (I'm not setup for Android dev on my MacBook at the moment). I'll do my best to fix up documentation to make this plugin's behavior as clear as possible.

I'm wondering what would be the best way to exercise this plugin other than the rather simplistic tests this PR adds. If we had more official plugins supporting background execution, I'd imagine that looking into having a dedicated device running applications triggering background execution events would be something worth doing, but other than that I'm not sure what we can do.

bkonyi · 2019-11-18T23:57:51Z

@collinjackson it looks like the tests are failing, but I'm not sure why... is the test being run with flutter test on Cirrus?

xster · 2019-11-21T04:06:04Z

Seems like the error on firebase test lab is

Permission Denial: starting instrumentation ComponentInfo{io.flutter.plugins.androidalarmmanagerexample.test/androidx.test.runner.AndroidJUnitRunner} from pid=18303, uid=18303 not allowed because package io.flutter.plugins.androidalarmmanagerexample.test does not have a signature matching the target io.flutter.plugins.androidalarmmanagerexample

bkonyi · 2019-11-22T01:02:26Z

So it looks like the timeout on Firebase is too aggressive for the e2e test to pass on API 28+ and we timeout. If you look at the logcat output, we do see an incrementCounter: 1 in the logs after the test has been cancelled.

@collinjackson @amirh, any suggestions?

collinjackson · 2019-11-26T00:42:22Z

So it looks like the timeout on Firebase is too aggressive for the e2e test to pass on API 28+ and we timeout. If you look at the logcat output, we do see an incrementCounter: 1 in the logs after the test has been cancelled.

@collinjackson @amirh, any suggestions?

I'm in favor of longer (5m?) timeouts to reduce flakiness if that actually causes the test to pass. We could also make the timeouts configurable per plugin but maybe that's overthinking it.

bkonyi · 2019-11-26T23:42:51Z

Finally got all the checks passing... Any objections to me landing this?

vkammerer · 2019-11-27T01:34:01Z

Really looking forward to the publication of this update!

…2193)

googlebot added the cla: yes label Oct 15, 2019

bkonyi requested review from matthew-carroll and mklim October 15, 2019 16:33

collinjackson self-requested a review October 15, 2019 19:32

collinjackson reviewed Oct 15, 2019

View reviewed changes

matthew-carroll reviewed Oct 15, 2019

View reviewed changes

mklim reviewed Oct 16, 2019

View reviewed changes

matthew-carroll reviewed Oct 17, 2019

View reviewed changes

collinjackson reviewed Oct 18, 2019

View reviewed changes

...le/android/app/src/main/java/io/flutter/plugins/androidalarmmanagerexample/MainActivity.java Show resolved Hide resolved

bkonyi added 11 commits November 5, 2019 12:44

[android_alarm_manager] migrate to the V2 Android embedding

dc19c71

Update change log and version

7323dac

Formatting

df1849f

Address review comments

2c71ccb

Formatting and added MainActivityTest.java

03519ea

Added tests

60f3fe0

Fixed test analysis failures

458d81e

Additional migration work

6a6cea7

Formatting

1e73ec4

Fixed publishable failures

6bd80ee

Update pubspec

010b7ea

bkonyi force-pushed the android_alarm_manage_v2_embedding branch from 908cd1d to 010b7ea Compare November 5, 2019 20:48

bkonyi added 2 commits November 5, 2019 12:57

Update Flutter version lower bound

2c28c5b

Add back authors

acc5d62

Formatting

cf85ae0

matthew-carroll reviewed Nov 6, 2019

View reviewed changes

matthew-carroll approved these changes Nov 14, 2019

View reviewed changes

Addressed comments

6a14770

bkonyi added 4 commits November 21, 2019 16:32

Resume background isolate for driver test

59a31ac

Fixed comment

c6b467b

Bump timeout

2663983

Formatting

b03c818

Bump Firebase timeout

e557d58

bkonyi added 4 commits November 25, 2019 16:53

Remove loop limit in test

d036c04

Formatting

50a38dc

Merge branch 'master' into android_alarm_manage_v2_embedding

f81f898

Fix pedantic analysis errors

0e4c639

bkonyi merged commit e61e9d4 into master Nov 27, 2019

bkonyi deleted the android_alarm_manage_v2_embedding branch November 27, 2019 21:42

bkonyi mentioned this pull request Nov 27, 2019

[android_alarm_manager] Update minimum Flutter version to 1.12.0 #2327

Merged

sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019

[android_alarm_manager] migrate to the V2 Android embedding (flutter#…

22a180e

…2193)

vkammerer mentioned this pull request Jan 8, 2020

android_alarm_manager plugin: second alarm doesn't fire when main view is opened after first alarm flutter/flutter#44264

Closed

vkammerer mentioned this pull request Feb 24, 2020

[firebase_messaging] Support Flutter 1.12 Android embedding firebase/flutterfire#2017

Closed


		private AtomicBoolean isIsolateRunning = new AtomicBoolean(false);

		private static PluginRegistrantCallback pluginRegistrantCallback;


		private static PluginRegistrantCallback pluginRegistrantCallback;

		public static void setPluginRegistrant(PluginRegistrantCallback callback) {

[android_alarm_manager] migrate to the V2 Android embedding #2193

[android_alarm_manager] migrate to the V2 Android embedding #2193

Uh oh!

Conversation

bkonyi commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

bkonyi commented Oct 15, 2019

Uh oh!

collinjackson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkonyi commented Oct 17, 2019

Uh oh!

bkonyi commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

collinjackson commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkonyi commented Oct 18, 2019

Uh oh!

Uh oh!

xster commented Nov 5, 2019

Uh oh!

bkonyi commented Nov 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-carroll left a comment

bkonyi commented Oct 15, 2019 •

edited

Loading

collinjackson left a comment •

edited

Loading

bkonyi commented Oct 17, 2019 •

edited

Loading

collinjackson commented Oct 17, 2019 •

edited

Loading

xster commented Nov 21, 2019 •

edited

Loading