Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hot Restart should be notified to program code so it can dispose of resources. #42679

Open
ditman opened this issue Jul 13, 2020 · 19 comments
Open
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P2 A bug or feature request we're likely to work on web-dev-compiler

Comments

@ditman
Copy link
Member

ditman commented Jul 13, 2020

I have a plugin/app that attaches a listener to the window.navigator.connection.onChange Stream.

However, when the app/plugin hot restarts, I don't have any opportunity of canceling my old listener before attaching again.

Since connection.onChange is a DOM global, I end up with callbacks firing multiple times (+1 after each hot reload).

Is there any way I can detect a Hot Restart from app/plugin code before it happens, so I can dispose of the resources I've taken?

See also: flutter/flutter#10437

(I have worked around this issue by writing directly to the onchange property of the underlying DOM object, but that is not "polite")

  • SDK Version: 2.9.0-21.0.dev.flutter-06cb010247 (be) (Tue Jul 7 08:14:51 2020 +0000) on "linux_x64"
  • OS: Linux (doesn't matter)
  • Browser: Chrome (doesn't matter)
@sigmundch sigmundch added area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P2 A bug or feature request we're likely to work on web-dev-compiler labels Jul 13, 2020
@jonahwilliams
Copy link
Contributor

Running any/arbitrary user code on hot reload/restart has the risk of stalling or creating deadlocks in the core development experience. That is, If dependencies can hook into hot reload/restart they can break it too.

Are there less intrusive options here? If the problem is resource cleanup, can we try a more targeted solution?

@ditman
Copy link
Member Author

ditman commented Jul 14, 2020

This problem is caused, probably in part by the architecture of the app that causes this.

What does/doesn't survive when hot restart happens? Maybe there's a better lifecycle hook than initState to attach that listener... (Whatever is "symmetrical" to dispose, I guess?)

@ditman

This comment has been minimized.

@ditman ditman closed this as completed Feb 27, 2021
@ditman
Copy link
Member Author

ditman commented Feb 27, 2021

I take the above back, this is still happening. I tried to use reassemble from flutter, but that doesn't seem to be called in my app :/

@ditman ditman reopened this Feb 27, 2021
@ditman
Copy link
Member Author

ditman commented Mar 3, 2021

/cc @ferhatb

@maks
Copy link

maks commented Oct 28, 2021

Sadly this problem is still an issue now, is not just for Dart on Web, is affecting more and more projects (see number of incoming issue links to the downstream bug on Flutter: flutter/flutter#10437) and is an issue for the growing use of FFI.

In my case, I'm using a library via FFI in a Flutter plugin that needs to be able to clean up resources, which is impossible with the current hot-restart behaviour, which leads to a much degraded developer UX of making hot-restart useless as hot-restart removes Dart side ref to the instance of the native libraries resources and it's not possible to re-initialise the native lib in that state, so the only thing I can do is hot-reload or full app stop and restart :-(

@pcrady
Copy link

pcrady commented Nov 22, 2021

Im running into this problem with websockets on web. See issue 93793 in flutter.

@ditman
Copy link
Member Author

ditman commented Apr 20, 2022

This looks like something that could be solved with a Finalizer (maybe)? Docs.

@sigmundch
Copy link
Member

@nshahan @natebiggs - Let's use this issue to consolidate the multiple threads we have on this topic.

We continue to see examples where developers need a hook to act during a hot restart.

A recent example in grpc/grpc-dart#718 highlights two problems: an open resource is not closed (mostly wasteful, similar to the websocket issue shared here) and an old callback that starts getting called with instances of the new code.

We have ideas that could improve the latter. Like we do with timers (where we keep them, but don't execute them after the hot-restart). We could do the same for async methods and callbacks that escape Dart (e.g. DOM callbacks), but it will come at a cost.

Even if we did that, the problem of leaking resources remains. Without a hook to act on a hot-restart some user journeys are forced to do a full reload instead (e.g. FFI case).

Seems so far we have a few ideas being tossed around to address this:

  • add a dart:* API to notify about hot-restart events
  • generalize the current service-extension mechanism (beyond what's done for flutter)
  • explore using finalizers

@ditman - I really like the idea of Finalizers and how it conceptually connects here! A hot-restart is effectively freeing up every instance in the Dart application, so it is a moment where finalizers could be invoked.

@nshahan @natebiggs - At first, I thought we couldn't do this in DDC because we don't control the heap. However, it seems feasible for us to save all finalizers in a weak structure that we can iterate and use and invoke on a hot restart. This would require changing our FinalizationRegistry patch to store and save finalizers over time. We'd need an iterable weak-map implementation using weakrefs underneath (like this example) so we don't leak memory as a result of this. What do you think?

@a-siva - does the VM by chance already run finalizers at this stage after a hot-restart?

@bkonyi - I was curious about your thoughts on using service extensions here. Are there other mechanisms available to us beyond service extensions for this? Ideally we'd like a way of sending a notification in the application space that comes from the service protocol, but not limit how many can listen to it (currently a service extension only allows for a single listener). Would it be feasible to expose a stream of events in dart:developer?

@bkonyi
Copy link
Contributor

bkonyi commented Jul 3, 2024

Hm, this really depends on how hot restart works on the web.

In the context of the native Flutter engine (the standalone Dart VM doesn't have the concept of hot restart), hot restart results in the original isolates being completely destroyed and replaced by new isolates. This would result in the service extensions registered through dart:developer being destroyed along with the isolate the extension was registered in. This means that any event used to clean up resources after a hot restart needs to be sent before the hot restart occurs and the runtime needs to wait for the cleanup to complete before actually triggering the restart.

As you've pointed out, registering a service extension with a given name via dart:developer can only be done once per isolate. This means that all cleanup would either need to be done directly in the singular service extension callback or there needs to be a mechanism to add callbacks to some list that the service extension can iterate over later. An event based solution here probably wouldn't work since the runtime will need to wait for the callbacks to complete to ensure cleanup is done before performing the restart.

If it's possible to make use of finalizers, that's probably the most generic solution that won't require adding hooks for doing hot restart cleanup work. In general, since hot restart isn't part of the core VM service protocol, I'd like to avoid introducing anything hot restart related into the service protocol or dart:developer.

@sigmundch
Copy link
Member

Thanks @bkonyi! That totally makes sense.

... native Flutter engine (the standalone Dart VM doesn't have the concept of hot restart)

😮 off-topic, but I'm curious to learn why hot-restart is not in the Dart VM but only on the flutter engine?

... original isolates being completely destroyed and replaced by new isolates.

Do you know if finalizers are being invoked in this process? Or whether it is feasible to enforce?

If it's possible to make use of finalizers, that's probably the most generic solution ...

Agree! What gives me pause is that generally we don't promise that finalizers will always be invoked, especially at the end of a program run. If we can provide a guarantee that finalizers will be invoked on a hot-restart, then this provides a simple and generally available API.

@bkonyi
Copy link
Contributor

bkonyi commented Jul 3, 2024

😮 off-topic, but I'm curious to learn why hot-restart is not in the Dart VM but only on the flutter engine?

That's a good question, but I don't have a great answer. Hot restart was added before I started on the project, but I assume it was only added in the Flutter engine and not the standalone VM since: A) there's a high cost to stopping and rebuilding a Flutter application compared to re-running a Dart application and B) hot restart could be completely implemented by the Flutter engine using the Dart VM embedding APIs. I'm also not aware of any requests to add hot restart to the standalone VM, especially since there's unlikely to be many users of hot reload outside of Flutter.

Do you know if finalizers are being invoked in this process? Or whether it is feasible to enforce?

If it's possible to make use of finalizers, that's probably the most generic solution ...

Agree! What gives me pause is that generally we don't promise that finalizers will always be invoked, especially at the end of a program run. If we can provide a guarantee that finalizers will be invoked on a hot-restart, then this provides a simple and generally available API.

I'm not too familiar with the details related to finalizers, but I assume they should be invoked before isolate death. Maybe @dcharkes can give more details here.

@mosuem
Copy link
Member

mosuem commented Jul 5, 2024

Just a drive-by comment: I would prefer if users would not have to write package code such as for package:grpc with Flutter features like hot reload in mind..

@dcharkes
Copy link
Contributor

dcharkes commented Jul 5, 2024

I really like the idea of Finalizers and how it conceptually connects here!

Finalizers are not run on Isolate shutdown. So they cannot be used.
NativeFinalizers are run on Isolate shtudown. But currently Flutter doesn't shut down all isolates before starting new ones on hot-restart. flutter/flutter#75528 (comment)

does the VM by chance already run finalizers at this stage after a hot-restart?

As written above, Finalizers are not run, NativeFinalizers are. However they are run on isolate shutdown and FlutterEngine::Restart is not shutting down isolates before starting new ones.

hot restart results in the original isolates being completely destroyed and replaced by new isolates.

Unfortunately not in that order currently. The new isolates are started before the old ones being stopped.

Just a drive-by comment: I would prefer if users would not have to write package code such as for package:grpc with Flutter features like hot reload in mind..

I don't think this is possible. As soon as there is native code involved (via the OS: sockets, file handles; or via dynamic libraries: database handles, running machine learning queries, etc.) a developer cannot ignore the fact that Dart has a way to completely reset all state in the Dart objects.

@sigmundch
Copy link
Member

Thanks for the pointer to the other issue!

Finalizers are not run on Isolate shutdown. So they cannot be used.

Is is a non-goal to eventually run all finalizers on isolate shutdown?

I ask because we want something that is not FFI-specific. On the web, FFI is not available and developers are encountering this issue through other "native" APIs that are then exposed as Dart APIs (e.g. web sockets).

If we won't run all finalizers on isolate shutdown, maybe we can expose a way to opt-in? For example: Finalizer(callback, onShutdown: true) ?

FlutterEngine::Restart is not shutting down isolates before starting new ones

What's the current behavior for NativeFinalizers, are there still being run after the hot-restart finishes and the new isolates are running?

FWIW, on the web, we don't have an actual 'shutdown' operation, but I believe we can ensure that finalizers get executed first before the app is restarted.

@dcharkes
Copy link
Contributor

dcharkes commented Jul 9, 2024

Is is a non-goal to eventually run all finalizers on isolate shutdown?

Yes. Finalizers clean up things in the Dart heap. And the Dart heap is disappearing on isolate group shutdown.
NativeFinalizers clean up resources outside the Dart heap (c memory, database handles, file handles, sockets, ...) which should be run on isolate group shutdown (assuming that the embedder keeps running and possible has other Dart VMs or remaining/new isolate groups.

If we won't run all finalizers on isolate shutdown, maybe we can expose a way to opt-in? For example: Finalizer(callback, onShutdown: true) ?

This might be challenging for multiple reasons:

  • Shutdown tears down the context in which to run Dart code, so we'd have to run the finalizers before actually starting the shutdown.
  • We probably don't want to run any other user code besides the finalizers. exit(0); should definitely not run any more Dart code. And usually there's just a message queue for all async code that still needs to run. Finalizer callbacks are in that same queue.

Maybe it would be better to introduce JsFinalizers for web backends that use the JS-interop to call a JS callback instead of Dart code.

I ask because we want something that is not FFI-specific. On the web, FFI is not available and developers are encountering this issue through other "native" APIs that are then exposed as Dart APIs (e.g. web sockets).

With FFI we also expose things through Dart APIs. But internally we have access to underlying native callback, so we attach a NativeFinalizer hidden behind the API that users don't see.

The JsFinalizer would be similar to the NativeFinalizer in the sense that it is meant to finalize resources external to the Dart heap. And in the sense that it does a callback to 'native' code and thus does not execute Dart code or modify the Dart heap. WDYT?

cc @mraleph @lrhn who were involved in the Finalizer and NativeFinalizer design.

@lrhn
Copy link
Member

lrhn commented Jul 9, 2024

A JsFinalizer sounds like a job for JS-interop.

I don't know how viable it is, if the JS heap survives, presumably finalizers don't need to run.
If it doesn't, because you navigate away, they can't run any more.
The Dart Finalizer on the web is implemented using the native JS finalizer, so I can't see what better alternative there is for JS interop to use instead, to minimize native finalizers.

@sigmundch
Copy link
Member

Shutdown tears down the context in which to run Dart code, so we'd have to run the finalizers before actually starting the shutdown.

Yeah, at that point I feel it is not different from exposing a way to enqueue callbacks for hot-restart (like flutter/flutter#130662). I was honestly surprised when I learned that hot-restart is not in the VM but only in the flutter. The web's hot-restart is used outside flutter, so it is part of our Dart offering. Such API would be easier to provide if it was a Dart feature everywhere.

Maybe it would be better to introduce JsFinalizers for web backends that use the JS-interop to call a JS callback instead of Dart code.

Indeed, I was starting to ponder about that, and it seems reasonable if we can't or don't want to expose a pre-hot-restart callback mechanism.

I sense that besides the interop-based finalizer, we'd need to define native finalizers ourselves for the native APIs we expose (say in dart:html). That would bring us closer to the model used by the VM with dart:io + FFI.

That said, some challenges on our end are that:

  • We currently have no notion of which, of all the native APIs we provide, need a finalizer (e.g. should websockets be closed? should DOM event listeners be disposed?)
  • We have no way to prevent Dart code from being run: you can always export Dart code as a native JS function.

I don't know how viable it is, if the JS heap survives, presumably finalizers don't need to run.
If it doesn't, because you navigate away, they can't run any more.
The Dart Finalizer on the web is implemented using the native JS finalizer, so I can't see what better alternative there is for JS interop to use instead, to minimize native finalizers.

We are in a third scenario: the heap survives and native finalizers need to run. That's because hot-restart on the web saves time by preserving the code and program structure, which it does by keeping the heap alive. As such, we can't rely on GC-based JS finalizers. We don't control when GC is run, and we need to guarantee those native finalizers run before the application is restarted.

I believe here we'd need a separate set of finalizers that we run by hand. To prevent leaks, I hope we can create some form of Iterable WeakMap to store pending native finalizers.

@dcharkes
Copy link
Contributor

dcharkes commented Jul 9, 2024

We have no way to prevent Dart code from being run: you can always export Dart code as a native JS function.

Of course people can try to call Dart code with FFI callbacks from native finalizers as well, but that is explicitly stated undefined behavior.

As such, we can't rely on GC-based JS finalizers. We don't control when GC is run, and we need to guarantee those native finalizers run before the application is restarted.

I believe here we'd need a separate set of finalizers that we run by hand. To prevent leaks, I hope we can create some form of Iterable WeakMap to store pending native finalizers.

Exactly the same is true for native finalizers. We keep a weak datastructure in the isolate so that we can iterate through the native finalizers from the C++ VM code on isolate (group) shutdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P2 A bug or feature request we're likely to work on web-dev-compiler
Projects
None yet
Development

No branches or pull requests

9 participants