shorebirdtech / shorebird

Code Push for Flutter and other tools for Flutter businesses.
https://shorebird.dev
Other
2.17k stars 129 forks source link

feat: enable advanced users to implement patch-install-without-restart #2350

Open alexmercerind opened 1 month ago

alexmercerind commented 1 month ago

TL;DR

Currently, shorebird_report_launch_start is not exposed from libflutter.so. Please expose it from the native library.

Description

We have a use-case where we want to use the downloaded patch without having to restart the entire process (& only restarting i.e. create & replace existing Flutter engine). We want to avoid restarting the entire process i.e. how package:restart_app does it, since it has few drawbacks:

I can confirm that the newly created Flutter engine loads the new patch after shorebird_report_launch_start has been called. On Android specifically (similar on iOS), we are using following code (through platform channels) to load the new patch:

runCatching {
    val flutterLoader = FlutterLoader(FlutterJNI.Factory().provideFlutterJNI())
    // Internally calls [shorebird_report_launch_start] through JNI.
    flutterLoader.startInitialization(activity!!.applicationContext)
    flutterLoader.ensureInitializationComplete(activity!!.applicationContext, null)
}.onFailure {
    Log.e(TAG, "Unable to initialize the Flutter runtime.", it)
}

// [FlutterActivity.recreate] is sufficient to re-create a new Flutter engine.
// Dart VM's main() is executed again.

activity?.recreate()

Instead of calling above Flutter embedding APIs to re-initialize the Flutter runtime (& thus Shorebird), we'd ideally like to call shorebird_report_launch_start directly to load the new patch. This have several benefits:

Additional Context

I have only added details for the Android side of things, please consider the same for iOS.

Related discussion:

eseidel commented 1 month ago

Interesting. The reason why we don't offer "hot restart" of sorts is that we can't know if plugins will manage the restart correctly. We could certainly expose some sort of advanced api to allow sophisticated hybrid apps to manage their own restart of Flutter.

It sounds like you already are managing your own Flutter Engine instance e.g. https://api.flutter.dev/javadoc/io/flutter/embedding/engine/package-summary.html?

alexmercerind commented 1 month ago

Greetings,

Thanks a lot for your reply.

Yes, I recreate Flutter engine using a plugin (similar discussion: https://github.com/flutter/flutter/issues/127409). I have pushed it into a repository for your reference: https://github.com/farmako/flutter_restart

I have created a demo project that uses Shorebird & above plugin: https://github.com/alexmercerind/test

The simple project contains two buttons, "Restart" & "Update", which:

  1. Restart Flutter engine.
  2. Download latest Shorebird patch.

I can confirm it works as intended on Android... I can't say the same for iOS.


On iOS, the very first patch did get loaded after clicking "Restart" button followed by "Update". However, subsequent patches only loaded after process quit & relaunch. I can't really test now, since I was testing on my colleague's iOS device... will have to check on Monday likely.

eseidel commented 1 month ago

Impressive. Unfortunately, those are not public repos (if you had intended them to be), or at least the links 404 for me.

alexmercerind commented 1 month ago

Eek... The repositories are now public.

eseidel commented 1 month ago

To make sure I understand: the ask is that we expose a hook for you to know when to call this on iOS? I believe you can know this today based on the return value of isNewPatchReadyToInstall?

https://pub.dev/documentation/shorebird_code_push/latest/shorebird_code_push/ShorebirdCodePush/isNewPatchReadyToInstall.html

alexmercerind commented 1 month ago

Ideally, I want some method to "install" the downloaded patch (before creating new Flutter engine). I think shorebird_report_launch_start does it.

During splash screen, if a new update is there... download & restart Flutter engine.

Future<void> _checkForUpdates() async {
  final isUpdateAvailable = await shorebirdCodePush.isNewPatchAvailableForDownload();
  if (isUpdateAvailable) {
    await shorebirdCodePush.downloadUpdateIfAvailable();
    await restart();
  }
}
eseidel commented 1 month ago

So patches get inflated onto disk. Once they're on disk, they're "installed" and Shorebird's Flutter Engine will use any installed patch on boot.

All shorebird_report_launch_start does is record in our state.json file that the launch is happening (so we can detect if a launch failed due to a crash and mark a patch "bad" if we see a launch started from a patch but never completed).

The pseudo code you wrote above looks like it could work? Can you help me understand what error you're seeing?

eseidel commented 1 month ago

I should note re: patches installed on disk being used. If patch-signing is in use the patch must be signed or Shorebird will reject it on boot: https://docs.shorebird.dev/guides/patch-signing/. Patch signing is currently opt-in, we expect to make it on by default eventually once we have the ability to create/manage keys for those who do not wish to manage their own. I don't get the sense you're using patch signing here, but thought I should mention regardless.

alexmercerind commented 1 month ago

Interesting. I had to add this additional code on Android to get the newly created Flutter engine use the latest patch. It calls flutter::FlutterMain::Init -> flutter::ConfigureShorebird -> shorebird_report_launch_start.

The comment of shorebird_report_launch_start says:

Tell the updater that we're launching from what it told us was the next patch to boot from. This will copy the next boot patch to be the current_boot patch.

Which I believe is exactly what I needed i.e. have the newly created Flutter engine use the next boot patch (_"will copy the next boot patch to be the currentboot patch").


The restart method from plugin isn't "rebooting" the process, only recreating the Flutter engine.

eseidel commented 1 month ago

shorebird_report_launch_start is used for internal bookkeeping by the updater: https://github.com/shorebirdtech/updater/blob/main/library/src/updater.rs#L428

Normally it's called by the engine: https://github.com/shorebirdtech/engine/blob/a72fb61b0f4211313cabc1395e7ba062c5fa176a/shell/common/shorebird/shorebird.cc#L163

On android that's called from flutter_main.cc: https://github.com/shorebirdtech/engine/blob/a72fb61b0f4211313cabc1395e7ba062c5fa176a/shell/platform/android/flutter_main.cc#L134

I think you're correct that we haven't really though through what calls would be needed.

If shorebird_report_launch_success isn't called after shorebird_report_launch_start the updater library will be confused (and mark the patch bad). I'm not sure how the updater library would behave if shorebird_report_launch_start is called after shorebird_report_launch_success is, it's not a case we've tested so far.

I think this would need more thinking from us to provide APIs to support this. But yes, I understand at least one of the problems for now.

eseidel commented 1 month ago

Essentially this bug is asking for https://github.com/shorebirdtech/shorebird/issues/741, except is offering to implement much of it, we'd just need to clean up our updater code a bit to make sure it's OK with multiple patches running in a single lifetime.

alexmercerind commented 1 month ago

Thanks a lot for all your help & details about the project.

The setup in the test project I provided works perfectly on Android. However, it only worked for the first patch on iOS. Subsequent patches on iOS e.g. color change were only visible after app reboot (& not with "Restart" button).

eseidel commented 1 month ago

iOS uses a static for holding the patch after load. We'd need to get rid of that hack: https://github.com/shorebirdtech/engine/blob/shorebird/dev/runtime/dart_snapshot.cc#L65

alexmercerind commented 1 month ago

Finally it makes sense. I think something like this should make it work:

diff --git a/runtime/dart_snapshot.cc b/runtime/dart_snapshot.cc
index 312fb7c64f..f021a36967 100644
--- a/runtime/dart_snapshot.cc
+++ b/runtime/dart_snapshot.cc
@@ -69,6 +69,7 @@ static std::shared_ptr<const fml::Mapping> SearchMapping(
     // become invalid.
     // "leaked_elf" is meant to indicate that we're not freeing the ELF.
     static Dart_LoadedElf* leaked_elf = nullptr;
+    static std::string leaked_elf_patch_path = "";
     // The VM Snapshot is identical for all binaries produced by a given version
     // of Dart.  Our linker checks this and will fail to link if ever the VM
     // snapshot changes.
@@ -76,7 +77,11 @@ static std::shared_ptr<const fml::Mapping> SearchMapping(
     const uint8_t* ignored_vm_instrs = nullptr;
     static const uint8_t* isolate_data = nullptr;
     static const uint8_t* isolate_instrs = nullptr;
-    if (leaked_elf == nullptr) {
+    if (patch_path != leaked_elf_patch_path) {
+      if (leaked_elf != nullptr) {
+        Dart_UnloadELF(leaked_elf);
+        leaked_elf = nullptr;
+      }
       const char* error = nullptr;
       // vmcode files are elf files prefixed with a shorebird linker header.
       auto elf_mapping = GetFileMapping(patch_path, false /* executable */);
@@ -88,6 +93,7 @@ static std::shared_ptr<const fml::Mapping> SearchMapping(
                                 &isolate_data, &isolate_instrs,
                                 /* load as read-only, not rx */ false);
       if (leaked_elf != nullptr) {
+        leaked_elf_patch_path = patch_path;
         FML_LOG(INFO) << "Loaded ELF";
       } else {
         FML_LOG(FATAL) << "Failed to load ELF at " << patch_path
eseidel commented 1 month ago

Exactly. That's also a hack (not your fault), but probably would unblock you, yes.

eseidel commented 1 month ago

I'm quite impressed, btw. This stuff is not easy to hack on. I think we can land something like that patch next week if it would unblock you. Feel encouraged to reach out to us over Discord as well if we forget: https://discord.gg/shorebird.

alexmercerind commented 1 month ago

Thanks a lot for your help. It'll be awesome if we can have some similar workaround for iOS.

I've been part of your Discord pretty much from the beginning. Great to see the project progressing!

alexmercerind commented 3 weeks ago

Greetings @eseidel,

It will be awesome if we can to land the said workaround in Shorebird's Flutter engine.

We'll be able to roll out Shorebird in our application's flow.

Thanks!

eseidel commented 3 weeks ago

My hesitancy has been that this will be an incomplete fix.

  1. It's incomplete because the "right" way to fix this is for us to move away from this "leak" hack entirely. I'm not 100% sure yet what that looks like... if it means either moving to use the embedder callbacks for loading the snapshot, or if it means returning some sort of custom Mapping that holds onto the loaded snapshot or otherwise allows us to tie the lifetime of the snapshot load at least to the Engine/Shell.
  2. Even if we had the right C++ fix, I'm still not sure this will work reliably for you. There is the previously mentioned case of needing to make sure plugins are aware of the "restart" of the FlutterView (which maybe your advanced setup already covers), but also cases like making sure to reset the keyboard state (possibly accessibility states or other platform-level states?). Which leads me to believe that even if we did this small workaround, it wouldn't be maintainable (which is maybe ok?) for either us (we wouldn't know when we could change this code since we don't currently support this "feature") or you (it could cause crashes/strange behavior in the wild with the keyboard, plugins, etc.)

Which is why every time I've come to this mail in my inbox over the last week I've not taken action.

alexmercerind commented 3 weeks ago

Thanks a lot for your response @eseidel. I genuinely understand your position & reasoning.

I'll also want the Shorebird's Flutter engine to get rid of the "leak" hack & static variables entirely, I also really look forward. In an ideal situation, I want the (newly) created Flutter engine to use the latest patch instead of the one currently initialized in the static variable.

In our setup, I am not accessing any private Flutter or Shorebird API. The plugins also get registered correctly with the newly created Flutter engine. The rest of the lifecycle is tied directly to existing FlutterActivity or FlutterViewController implementations from Flutter, which I do not modify. The setup may already be similar to the add-to-app feature, which I believe Shorebird already supports.


For the time being, it'll be greatly helpful to me, if I can utilize a custom self-compiled version of Flutter engine with Shorebird CLI one way or another.