Add hot restart hooks to support dart:ffi

Consider a Dart usage of a C api through ffi. A common pattern is the usage of context objects that must be explicitly setup and torn down for a given thread. ( This is simplified from an example program I wrote using https://sol.gfxile.net/soloud/ )

final lib = ffi.DynamicLibrary.open('some.dll');

class SoundSystem {
  ffi.Pointer _audioContext = lib.createContext();

  SoundSystem() {
    lib.setupContext(_audioContext);
   }
   
   void playSound() {
     lib.playSound(_audioContext);
   }
   
   void dispose() {
     lib.destroyContext(_audioContext);
   }
}

While this works well enough when performing a hot reload, but a hot restart may cause a native crash as recreating the SoundSystem with cause another audio context to be created without destroying the previous one.

During normal program execution this isn't a problem, since the dispose can be connected to the correct part of the application teardown logic ( or the handled by the OS as the program exits).

Trying to do something like teardown the entire widget tree may solve this problem, but would be expensive as well as a large behavior change with unintended consequences. Instead, I would propose adding a binding hook that allows developers/library authors to run callbacks before the hot restart is performed. The risk should be lower since the feature is opt-in, and failures could be ignored since the isolate would be destroyed anyway (though detecting these failures without using something like a timeout will be tricky).

For example, the above code could be modified like:

final lib = ffi.DynamicLibrary.open('some.dll');

class SoundSystem {
  ffi.Pointer _audioContext = lib.createContext();

  SoundSystem() {
    if (kDebugMode) {
      SomeBinding.instance!.addHotRestartTeardown(dispose);
    }
    lib.setupContext(_audioContext);
   }
   
   void playSound() {
     lib.playSound(_audioContext);
   }
   
   void dispose() {
     lib.destroyContext(_audioContext);
   }
}

This won't work if the library is trying to be platform agnostic though - which means we might need to consider either dart level support or a convention around using a dart:developer API. For example, something like

import 'dart:developer';

final lib = ffi.DynamicLibrary.open('some.dll');

class SoundSystem {
  ffi.Pointer _audioContext = lib.createContext();

  SoundSystem() {
    if (kDebugMode) {
      registerExtension('dart.ext.hotRestartTeardown', dispose);
    }
    lib.setupContext(_audioContext);
   }
   
   void playSound() {
     lib.playSound(_audioContext);
   }
   
   void dispose() {
     lib.destroyContext(_audioContext);
   }
}

Unfortunately I don't think that will work out of the box, since the various different libraries would stomp on eachother with different callbacks

@Hixie

FYI @Hixie @zanderso @dcharkes for thoughts.

Having a way to register a callback that is invoked just before a hot restart SGTM.
We should make sure to wrap each such callback in a try/finally block so that failures don't block the restart.
Such callbacks should presumably be asynchronous. I think it'd be fine to call all of them at once and then await all their returned Futures.
We should apply some timeout (like 5 seconds) to that wait, and if that expires print a message saying that hot restart is taking a long time (maybe each callback can have a label and we'd print the labels of any stalled callbacks in the message), providing some mechanism to continue with the hot restart regardless.

In the dart:io implementation (which is similar to clients of dart:ffi in that it wraps a lot of native APIs), whenever a native resource is handed to Dart code, it has a native finalizer attached to it, e.g. to call close() on a file descriptor as a backstop to prevent leaks. When working on dart:io, as a library author, I found that that defensive approach was preferable to hunting down missing dispose() calls in client code. Given some way to attach native finalizers to dart:ffi Pointer types, those finalizers would run when the main isolate is torn down during a hot restart.

A hot restart is also not the only time when an isolate might be be destroyed unexpectedly. See https://api.dart.dev/dev/2.13.0-4.0.dev/dart-isolate/Isolate/kill.html. If a parent isolate kills a child isolate, the child isolate will have no opportunity even to run service extension callbacks. So the only cleanup that can happen is running the finalizers on the native side.

However, it seems like it might be difficult to require dart:ffi users to provide a shim library for attaching a native finalizer for every library they'd want to use, and there might be other problems with exposing the finalizer API from dart_api.h. But cleaning up native resources whenever an isolate is torn down, not just due to a hot restart, is probably what we want. So more like:

registerExtension('dart.ext.isolateTeardown', dispose);

@mraleph

Also /cc @mraleph

Related issue for hot-reload #59061.

@zanderso

@zanderso I'm somewhat ignorant about the finalizer APIs in dart, but would this be something that is feasible to use with a library that is only available as a dll, or would it require authors to do some "wrapping" of the existing native APIs?

@dcharkes

@dcharkes we actually already have this for hot reload in flutter, you can use reassemble method provided your native API is connected to the widget tree in someway.

@zanderso

@zanderso I'm somewhat ignorant about the finalizer APIs in dart, but would this be something that is feasible to use with a library that is only available as a dll, or would it require authors to do some "wrapping" of the existing native APIs?

Yeah, I mentioned this above, but my understanding is that a wrapper/shim library would be required to attach the native finalizer before returning an object handle to Dart code. Putting my thoughts a different way:

Ideally dart:ffi has a native-side API (not necessarily the API currently in dart_api.h) for registering callbacks for cleaning up native resources when the Dart side drops all references (e.g. when an Isolate is getting torn down).
I suspect (1) is too hard, but an improvement would be the ability to add Dart-language cleanup hooks not only for hot restart, but whenever an Isolate is getting torn down. This won't cover as many cases as (1), but still better than nothing.

I could see that working in cases where a native library is written for the purpose of use in flutter. For my example usage I would probably just comment out the sound library when I'm not testing it, rather than write another wrapper.

@jonahwilliams

I ran into the very same issue that @jonahwilliams did, in my case trying to use via FFI a small library that uses MiniAudio as its audio backend that I had made. And likewise, everytime I hot-restart a Flutter app (well at least testing on Linux desktop & Android) that uses it (via a Flutter plugin I built to expose the library) the whole app crashes from native-side code because it doesn't release the audio context.

So I thought that the new (in Dart 2.17) NativeFinalizer would now be the solution to this. BUT it doesn't seem to work for the hot-restart case as the finalizer doesn't seem to be called when a hot-restart occurs.

This is the code I'm using (irrelevant bits removed) based on the code example in the NativeFinalizer dartdoc:

class LibMSFAPlugin implements Finalizable {
  static final _finalizer = NativeFinalizer(_bindings.addresses.shutdownEngine.cast());

  /// Used to prevent double close and usage after close.
  bool _shutdown = false;

  Future<bool> init() async {
    final SendPort helperIsolateSendPort = await _helperIsolateSendPort;
    final request = _InitRequest();
    final completer = Completer<bool>();
    _initRequest = completer;
    helperIsolateSendPort.send(request);
    return completer.future;
  }

  void sendMidi(List<int> list) {
    //...
  }

  void shutDown() {
    if (_shutdown) {
      return;
    }
    _shutdown = true;
    _finalizer.detach(this);
    _bindings.shutdownEngine();
  }
}

and I'm just using ffigen to generate the binding, again as per docs, so I'm not sure if I'm doing something wrong or if there is a bug with NativeFinalizers and Flutter?
@dcharkes would you have some insight on this?

I should add that my app was also crashing on Hot-reload but thanks to @jonahwilliams tip above on reassemble adding this code has at least stopped the crashes on hotreload:

 @override
  void reassemble() {
    super.reassemble();
    print("reassembling state...");
    plugin.shutDown();
    plugin = LibMSFAPlugin();
  }

So I thought that the new (in Dart 2.17) NativeFinalizer would now be the solution to this. BUT it doesn't seem to work for the hot-restart case as the finalizer doesn't seem to be called when a hot-restart occurs.

As far as I know, hot restart does not force a garbage collection. Neither is a single garbage collection guaranteed to collect all unreachable objects. Only on VM shutdown are the native finalizers guaranteed to run. So relying on garbage collection here is not a valid approach for supporting hot restart. The question to ask here is: is hot restart conceptually a VM-shutdown and VM-start? Or is it only a shutdown of all existing isolates? I've filed dart-lang/sdk#50016 to investigate this.

A quick test makes allocatedCounter increase by one on every hot restart.

This assessment is not entirely correct. It reproduces on MacOS and Android. The Linux implementation actually works perfectly fine with NativeFinalizers! (Repro: https://github.com/dcharkes/test_hot_restart ) So it seems that not being able to rely on NativeFinalizers is a per OS implementation problem.

@jonahwilliams

A hot restart should stop/kill all existing isolates and clear state (such as isolate name server registry)

I can repro this to be an issue.

Tested on:

$ flutter --version
Flutter 3.16.0-12.0.pre.63 • channel master • [email protected]:flutter/flutter
Framework • revision 88f302913a (3 days ago) • 2023-10-16 04:49:56 -0400
Engine • revision 15dee70086
Tools • Dart 3.3.0 (build 3.3.0-27.0.dev) • DevTools 2.28.1

Linux, MacOS, and iOS.

https://github.com/dcharkes/test_hot_restart

only a single isolate
NativeFinalizers get run before hot restart (all good 👌 )

Main isolate shutdown by

  pc 0x000000010e8bc458 fp 0x00000003092004b0 dart::Profiler::DumpStackTrace(bool)+0x28
  pc 0x000000010ebc1d87 fp 0x00000003092009f0 Dart_ShutdownIsolate+0x37
  pc 0x000000010e71b16d fp 0x0000000309200a20 flutter::DartIsolate::Shutdown()+0x5d
  pc 0x000000010e7283dd fp 0x0000000309200a90 flutter::RuntimeController::~RuntimeController()+0x9d
  pc 0x000000010e72859c fp 0x0000000309200ab0 flutter::RuntimeController::~RuntimeController()+0x1c
  pc 0x000000010e650d25 fp 0x0000000309200c70 flutter::Engine::Restart(flutter::RunConfiguration)+0x95

https://github.com/dcharkes/test_hot_restart/tree/extra-isolate

a helper isolate
NativeFinalizers get run after hot restart (the hot restart code races with the shutdown 💥 )

Helper isolate is shut down by

  pc 0x000000010e8bc458 fp 0x000000030a0c1900 dart::Profiler::DumpStackTrace(bool)+0x28
  pc 0x000000010ebc1d87 fp 0x000000030a0c1e40 Dart_ShutdownIsolate+0x37
  pc 0x000000010e80be42 fp 0x000000030a0c1ea0 dart::MessageHandler::TaskCallback()+0x342

Instead, we should be killing all isolates in RuntimeController::~RuntimeController(). Relying on the message system shuts down the isolate too late. We need to synchronously wait until all isolates are shutdown to ensure the NativeFinalizers are run.

I assume we don't support Isolate.spawnUri in Flutter (we ship no sources inside a flutter app)? So we should only have a single isolate group.

If we only have a single isolate group we have multiple options:

Save the isolate group in the RuntimeController and introduce Dart_KillIsolateGroup as Martin suggested earlier, or
don't save the isolate group but use DartIsolateScope + Dart_CurrentIsolateGroup to find the isolate group and introduce Dart_KillIsolateGroup, or
introduce a Dart_GetIsolateGroup(isolate) and Dart_KillIsolateGroup, or
if we already keep track of a list of all isolates somewhere in the Flutter engine, let RuntimeController' refer to that list and loop over every isolate in that list in the destructor.

@jonahwilliams Any preferences? I can make a PR for the Dart SDK and Flutter engine. (Also, anyone else who needs to be looped in for feedback here?)

I assume we don't support Isolate.spawnUri in Flutter (we ship no sources inside a flutter app)? So we should only have a single isolate group.

I think that might happen to work in debug mode? Or maybe that was disabled. But it definitely does not work in profile/release mode.

RE: the options above, I don't think the engine keeps track of any isolate lists, and isolates can come and go via Isolate.run/compute. So any options that involve the engine tracking that state is going to be complicated. If Dart already knows the isolates to shutdown I think I'd prefer to delegate that to Dart.

When a flutter user decides to restart (instead of reload) it should ensure all isolates of an isolate group are killed - otherwise it will lead two two different isolate groups (one with old code and one with new code) that cannot really communicate with each other anymore - see #124546 (which this issue is proposing to fix).

Though there's a complication: Flutter allows creating multiple UI isolates (see docs.flutter.dev/add-to-app/multiple-flutters). There's a question whether a hot-restart should affect one or all UI isolates.

If it effects all isolates: The flutter engine would need to ensure other UI isolates are also shutdown. But since all UI isolates are multiplexed on the same thread, very special care would need to be taken to not create deadlocks (as a e.g. synchronous Dart_KillCurrentIsolateGroup() API would create, as it's based on sending kill signals to all isolates and relying on them responding to messages). The native (java/obj-c/...) side may (???) also need to get notified about this (as it assumes different UI isolates still exist to render different views)
If it effects only one UI isolate: Someone would need to keep track all the helper isolates created from that UI isolate vs helper isolates belonging to other UI isolates. It would also lead to the issue of inter-isolate port communication where isolate cannot talk custom messages to each other anymore after restart - but the assumption would probably be that the different UI isolates are entirely unaware of each other.

(I wanted to bring this multiple-UI-isolate scenario here, because it's very uncommon and probably not very well tested - at least with hot-restart)

I think that we have briefly discussed the issues of hot reload and hot restart when there are multiple engines, and concluded that 1) The tooling doesn't handle this correctly today and 2) We're unsure what the right approach would even be (do users expect the hot reload/restart to only effect the entrypoint they are "running"?

I think that using multiple engines in a development environment is unlikely, and I wouldn' worry about it beyond making sure we don't explode.

I think that using multiple engines in a development environment is unlikely, and I wouldn' worry about it beyond making sure we don't explode.

Given that the current behavior is weird with regard to messages, and that adding a synchronous Dart_KillCurrentIsolateGroup() could lead to deadlocks, should we consider refusing to hot restart all together if there are multiple engines / multiple UI isolates? That would at least be clear behavior for users. (I'd need to check how we would detect multiple engines, I don't know the code by heart.)

I don't feel like I'm familiar enough with the multiple engines case to really make a call, but if the alternative is deadlocks then refusing to hot restart sounds reasonable.

I don't feel like I'm familiar enough with the multiple engines case to really make a call, but if the alternative is deadlocks then refusing to hot restart sounds reasonable.

Who is more knowledgable w.r.t. multiple engines?

@gaaclarke

I think that we have briefly discussed the issues of hot reload and hot restart when there are multiple engines, and concluded that 1) The tooling doesn't handle this correctly today and 2) We're unsure what the right approach would even be (do users expect the hot reload/restart to only effect the entrypoint they are "running"?

Flutter's big selling point is fast development time via hot-reload/hot-restart. So not supporting hot-reload/hot-restart at all for add2app / multi engine seems very harsh - as that would make it really hard for anyone to use it (development cycle may be terrible).

The hot-reload support in the VM will reload all isolates in an isolate group (that's the only thing that makes sense) - so supporting hot-reload in multi-engine scenario would affect all UI isolates. But hot-reload is mostly a VM-internal operation and doesn't require much flutter-engine or native (read: java/...) changes.

Given these hot-reload semantics, one could argue that a hot-restart semantics should also affect all multi-engine UI isolates. The question would then be how to do that safely in the engine and whether the native side would need changes as well (read: Java engine API) or it only requires flutter-engine internal changes.

Who is more knowledgable w.r.t. multiple engines?

AFAIK it was driven by @gaaclarke wdyt?

@gaaclarke

AFAIK it was driven by @gaaclarke wdyt?

Wow, a lot of background here.

I agree with Jonah's assessment that we don't keep track of a list of isolate in the engine, so any solution shouldn't hinge on that.

I would expect hot-reload/hot-restart with multiple engines to have the same semantics as having multiple isolates. So, as Martin was saying, it effects all isolates (including the ones associated with engines).

So, when I implemented the feature I assumed that hot-reload/hot-restart were all handled under the Dart API so the implementation never explicitly made any decisions about it, nor was it necessary for implementing it.

We later found out that the tool does not handle the multi-engine case. I'm not very familiar with the tool but I assumed it is a matter of doing something like:

- GetGlobalEngine()->HotReload();
+ for (auto& engine : GetGlobalEngines()) {
+   engine->HotReload();
+ }

My understanding is we've never implemented something like that. It's worth noting that its omission is not accidental.

From reading through this long thread it sounds like hot-reload is good. We need to accommodate hot-restart which has hooks in the engine that tear down isolates, causing finalizers to execute which is actually the top line problem for this thread, right?

We want to call Dart_KillCurrentIsolateGroup but the problem is that Dart_KillCurrentIsolateGroup doesn't handle the case where multiple isolates are running on the same thread. It seems to me that what you'd want to do is to loop over all the engines and have them kill their isolate. Once all the engine isolates are dead you could call Dart_KillCurrentIsolateGroup at which point the bookkeeping should be easier since all of the isolates in the group are killed and you won't need signals or thread synchronization. Calling Dart_KillCurrentIsolateGroup while another engine is relying on it doesn't make sense anyways.

Taking a step back, I would solve the top line issue first for single engines, then solve it for multi-engine usage because there seems to be a gap with tooling anyways wrt to multiple engines and the top line issue effects a lot of single engine users.

Anything else I can help with? There is a lot in this thread =)

@gaaclarke

It seems to me that what you'd want to do is to loop over all the engines and have them kill their isolate. Once all the engine isolates are dead you could call Dart_KillCurrentIsolateGroup at which point the bookkeeping should be easier since all of the isolates in the group are killed and you won't need signals or thread synchronization. Calling Dart_KillCurrentIsolateGroup while another engine is relying on it doesn't make sense anyways.

That indeed sounds like a simpler solution. Thanks @gaaclarke!

Right, so that means we can't have a Engine::Restart that kills and immediately starts an isolate again.

bool Engine::Restart(RunConfiguration configuration) {
  TRACE_EVENT0("flutter", "Engine::Restart");
  if (!configuration.IsValid()) {
    FML_LOG(ERROR) << "Engine run configuration was invalid.";
    return false;
  }
  delegate_.OnPreEngineRestart();
  runtime_controller_ = runtime_controller_->Clone(); // Kills isolate
  UpdateAssetManager(nullptr);
  return Run(std::move(configuration)) == Engine::RunStatus::Success; // Starts isolate
}

https://github.com/flutter/engine/blob/4da3e5b12540458b82ff1e7ba733d08f67977042/shell/common/engine.cc#L196-L206

The caller would need to of this would first need to Engine::Kill(/*whole_group=*/false) for all engines but one, then kill the isolate group Engine::Kill(/*whole_group=*/true) (we need one isolate from the isolate group to find the isolate group so we can kill it) and then Engine::Run again for all engines.

I'd have to dive into how the hot-restart request is wired so that it triggers a hot restart.

I'm unfamiliar with multi engine. Does it mean that someone else is writing C++ code that imports the Flutter C++ engine code and wires hot restart up manually? Where do I find examples of this?

Does our own C++ code always only have a single engine?

I'm unfamiliar with multi engine. Does it mean that someone else is writing C++ code that imports the Flutter C++ engine code and wires hot restart up manually? Where do I find examples of this?

Does our own C++ code always only have a single engine?

Here is an example that is doing it: https://docs.flutter.dev/add-to-app/multiple-flutters. The api is surfaced all the way up to users via objc, java, etc.

Here's how to think of "multi engine", the feature was called "lightweight engines". Customers wanted to have multiple engines in their app as they migrated existing apps to flutter piece-by-piece. The problem with that is that each engine has a lot of overhead with respect to memory and startup latency. Lightweight engines solves this problem by having multiple engines share the heaviest components, greatly reducing memory cost and startup latency. One of the heavy parts they share is the thread host. So, the engines will have 2 different dart isolates but they execute on the same thread. These shared resources are managed as std::shared_ptrs across engines. Is this helping?

Another use case: The drift package for managing sqlite/postgres connections has a transaction feature that locks the SQLite db while it is running. If you were in the middle of one (e.g. by placing a breakpoint), the next time the app tries to connect to it, the database will still be locked.

From: simolus3/drift#3558

Navigation Menu

Repository navigation

Add hot restart hooks to support dart:ffi #75528

28 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add hot restart hooks to support dart:ffi #75528

Description

Activity

jonahwilliams commented on Feb 5, 2021

Hixie commented on Feb 5, 2021

zanderso commented on Feb 5, 2021

zanderso commented on Feb 5, 2021

dcharkes commented on Feb 8, 2021

jonahwilliams commented on Feb 8, 2021

jonahwilliams commented on Feb 8, 2021

zanderso commented on Feb 8, 2021

jonahwilliams commented on Feb 8, 2021

maks commented on Sep 20, 2022

dcharkes commented on Sep 20, 2022

28 remaining items

dcharkes commented on Oct 16, 2023

dcharkes commented on Oct 19, 2023

jonahwilliams commented on Oct 19, 2023

mkustermann commented on Oct 30, 2023

jonahwilliams commented on Oct 30, 2023

dcharkes commented on Oct 30, 2023

jonahwilliams commented on Oct 30, 2023

dcharkes commented on Oct 31, 2023

mkustermann commented on Oct 31, 2023

gaaclarke commented on Oct 31, 2023

dcharkes commented on Oct 31, 2023

gaaclarke commented on Oct 31, 2023

Levi-Lesches commented on Jan 11, 2025

FMorschel commented on Jun 10, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions