Do plugins need to know about hot reload and hot restart?

eseidelGoogle commented 7 years ago

I think it would be an OK design choice if they didn't. But it's probably better if they do.

If we need to add new API to teach plugins about hot-reload or hot-restart we might need to do that sooner rather than later.

FYI @mravn-google

mravn-google commented 7 years ago

Ians suggestion in https://github.com/flutter/engine/pull/3722#issuecomment-305572460 may be a way to handle hot restart.

mravn-google commented 6 years ago

No new API needed.

bparrishMines commented 5 years ago

Reopening

cc @amirh

amirh commented 5 years ago

Some context as for why we re-open this issue: plugins may need to clean-up some state following a hot restart. E.g the camera plugin should make sure to close a previously opened camera following a hot restart (as the Flutter app driving the camera lost any state and is unaware that the device is open).

This was worked around in the camera plugin with a static initializer for the method channel that invokes an "init" method:

https://github.com/flutter/plugins/blob/c27f1f9a7b39af6d631fc5d333e7d8ddfddb453b/packages/camera/lib/camera.dart#L13-L14

This approach has a few drawbacks:

As the static initializer is lazily evaluated, it will only be executed the first time the field is accessed (so this is more of a "first use after hot reload" signal).
This prevents unit tests from setting a mock handler on the method channel(as the "init" invocation will happen before a mock handler is set and will crash).
Nothing prevents users from creating a separate method channel skipping the "init" invocation (while this can be abused to workaround the testability issue, we probably don't want users to do so).

An alternative workaround suggested in https://github.com/flutter/plugins/pull/1108#pullrequestreview-195981344 is to depend on the app developer to call an "initialize" method in the main() function. While that should work I think it's a little annoying for users (especially if you have multiple plugins that requires that hook).

I'm not sure why this issue was close saying "no new API needed" happy if someone can provide any additional context for that comment(maybe @sigurdm ?).

As a side note, for platform views we ran into a the same issue, and we did plumb the hot restart signal all the way up to the Android and iOS embedder(https://github.com/flutter/engine/pull/5929 https://github.com/flutter/engine/pull/6772), so exposing that to plugins should not be to hard (we just need to figure a good API).

sigurdm commented 5 years ago

As I remember at the time we had not really considered the problem with the lazy initialization, and where happy we could avoid having the plugin architecture needing a bigger api. I think I changed my mind since - it seems so generally useful for the plugin to know about hot restart.

p30arena commented 5 years ago

I think this issue is related: https://github.com/flutter/flutter/issues/30911 Does Hot Reload break the MethodChannel connection?

ppiatkowski commented 4 years ago

A plugin I'm working on does some cleanup in dispose() method. If hot restart does not call dispose() and there is no other way to get notified about hot restart then I'm going to be leaking resources. Please provide a way to cleanup my stuff during hot restart.

zeekhuge commented 4 years ago

Not sure, but notifying plugins about hot-reload does not look much useful, since reload is supposed to hold the app state. However, hot-restart should be notified to clean up state and resources. Can hot-restart simply "detach" and then "attach" the plugins ? Since we already have APIs for that, wouldn't have to introduce new one.

PS: This looks like a duplicate of #7160

maks commented 2 years ago

@ZeekHuge yes I think #7160 is the older issue for this, but there is alot of history now in this issue too.

@eseidelGoogle I think the number of incoming issue links above shows this has a widespread negative impact on developer UX and I think its going to become more widespread as more Flutter devs such as myself make more use of FFI to bind to native libraries, which is how I've run into this issue now.

Sadly I think this issue is stuck in limbo, leaving developers unable to use hot-restart anytime the app happens to make use of a native or platform resource that needs to be cleaned up in response to a hot-restart.

There seems to be a upstream Dart issue as well: https://github.com/dart-lang/sdk/issues/42679 but nothing seems to be happening there either.

eseidelGoogle commented 2 years ago

There is active work going on in the plugin API space (e.g. I just saw discussions of using thread pools, etc). I suspect this is on the table for @stuartmorgan, @blasten and others it just hasn't reached the top of the list (we have a lot we could do across relatively few people). I think the more concrete we can be about what effects we're seeking to fix (which behaviors, for which plugs), the easier to motivate this specific work. Thank you.

I agree with both of you, that it appears I filed the same issue twice! I have no objections to closing this in favor of another issue.

stuartmorgan commented 2 years ago

I think its going to become more widespread as more Flutter devs such as myself make more use of FFI to bind to native libraries

A solution to this issue wouldn't help with FFI. There is no "FFI API" the way there is a plugin API, and thus nowhere to put a hook.

If you want a solution for FFI, you should file a new issue with details of what you want to accomplish, as it would need to be entirely different (e.g., a Dart-side hook).

maks commented 2 years ago

Apologies, I should have been more specific with my comment. What I meant to say is that Flutter plugins (and even pure Dart packages) using FFI are in need of being able to detect a hot-restart as much as plugin's using platform channels.

Given this is a development-time only issue, I would be happy with as simple a mechanism as possible.

The effect I'm trying to fix is that I'm making use of a native library (eg. C, Rust, etc) which holds one or more resources (eg. sockets, bluetooth, audio etc) which need to be released on stop. On Linux desktop as an example, this happens if the hosting OS process is destroyed during debugging when the Flutter app is stopped, but does not happen during a hot-restart. I have to be honest and admit I don't know the details of what happens during a Dart hot-restart, but it seems to be that the running Isolate(s?) is killed and then started again? Any how, the effect is that Dart side references to the native library are lost, while the native library continues to run, potentially on its own thread(s).

To fix the above, I would request having some sort of lifecycle sync callback that can be registered by an app to be called prior to hot-restart taking place. This would allow me as the app author the chance to call any required close()/destroy()/stop(), etc on native libraries (or for that matter platform channel plugins) that I'm using to have them cleanup their resource usage. This then doesn't impact or require changes from existing native libraries as they would be expected to already be providing some sort of cleanup resources API and platform channel plugins do not need to be aware of this API, its just something a plugin user knows to make use of in order

I know this can potentially block hot-restart but as the app author I will have had to explicitly regsiter for this callback and then do something to block that callback completing so thats on me as the app author, plus I've already experienced existing bugs that cause hot-restarts to stall so its not something that would block dev work, but it would at least give app authors that are making use of such native libraries and platform channel plugins the effective use of hot-restart again.

Ideally I would expect this being done upstream in Dart because hot-restart is a Dart mechanism and there is an already an issue for it but I understand that pure Dart usage is a niche usage compared to Flutter so I would be happy just for a Flutter specific implementation.

nt4f04uNd commented 2 years ago

This one also looks duplicated https://github.com/flutter/flutter/issues/62678

stuartmorgan commented 2 years ago

@maks I understand the problem you want to solve; I'm saying that while it's closely related to this issue, it would have a completely different solution that what this issue envisions, so should be filed separately so that it can be triaged and discussed separately.

Notably:

fixing this issue would not help with FFI, and
even if the thing you wanted (a Dart-side callback) existed, it wouldn't necessarily mean that we wouldn't also want to fix this issue, because having every plugin have to manually plumb a method channel call for hot restart cleanup vs having it simply be a standard callback in the existing plugin API would be added boilerplate, and would needlessly introduce more overhead to something that we want to be as performant as possible.

maks commented 2 years ago

Sorry @stuartmorgan my explaining of the problem was in response to @eseidelGoogle comment:

think the more concrete we can be about what effects we're seeking to fix (which ...

I'm not sure I agree with you on point 2 though, while it would be useful not to have this functionality duplicated across all plugins, its not just plugins, there is no reason why Flutter apps cannot be using FFI directly without a plugin and hence have the same need to be notified of when a hot-restart occurs.

But given the above, I think you are quite correct in that this is not what this issue is about, as its title is specifically referring to plugins, so I realise now that what I'm asking for is actually what is covered in the Dart issue rather than this one, so I'll move my further discussion there.

Piero512 commented 2 years ago

and would needlessly introduce more overhead to something that we want to be as performant as possible.

If there's a way to tell the user that their hot restart experience is slow because of a plugin not finalizing their dispose callback fast enough, then I guess it would help in diagnosing the problem.

radvansky-tomas commented 2 years ago

Also adding web event listeners html.document.addEventListener('visibilitychange', _webVisibilitychange);

Will add them again on hot restart, without any way of clearing them before!

ryanheise commented 2 years ago

That is a good point, @radvansky-tomas .

Only Android currently has an elegant solution where you can add a lifecycle listener to the engine on the platform side, and listen to when the engine is restarted. (Not perfectly elegant, though, since the method FlutterPluginBinding.getflutterEngine has been deprecated).

The solution is:

public void onAttachedToEngine(FlutterPluginBinding binding) {
    @SuppressWarnings("deprecation")
    FlutterEngine engine = getFlutterEngine();
    engine.addEngineLifecycleListener(new EngineLifecycleListener() {
        public void onPreEngineRestart() {
            // Do cleanup here. E.g. kill active audio players
        }
        public void onEngineWillDestroy() { ... }
    });
}

I think this is the type of solution we need on the other platforms.

There is a workaround demonstrated in the video_player plugin which is to initiate the cleanup from the dart side by implementing a cleanup method on the platform side of the plugin. If you call it from the Dart side during the isolate's startup, then it will be called on each hot restart - but also inconveniently on the very first engine load when there was in fact no hot restart.

Even accepting that caveat, a different approach will be needed on web, so we are better off having some sort of hot restart lifecycle listener on all platforms.

jvictorsoto commented 1 year ago

Its 2023 and still not fixed...

LeoSandbox commented 1 year ago

Is there any update on this? I am coming from this issue: bluefireteam/audioplayers #1120

javiermrz commented 1 year ago

Any update? I'm ending up with several websocket connections, window listeners, and so on... (web)

marianhlavac commented 9 months ago

This is great. 6 years old issue and basically the only reaction from Flutter team is marking comments as "off-topic" when people urges that this issue should be taken care of. Way to go.

stuartmorgan commented 9 months ago

@marianhlavac Please take a moment to review https://github.com/flutter/flutter/wiki/Issue-hygiene#do-not-add-me-too-or-same-or-is-there-an-update-comments-to-bugs, https://github.com/flutter/flutter/wiki/Issue-hygiene#escalating-an-issue-that-has-the-wrong-priority, and https://github.com/flutter/flutter/blob/main/CODE_OF_CONDUCT.md

marianhlavac commented 9 months ago

@stuartmorgan Thank you. I've also took a moment to read about the priorities, and then listed the current P0+P1 issues to learn that the priority rules seems to be applied inconsistently, so not sure what does this say about the rest of the guidelines. I went through the first few of P0+P1 supposedly 'more important' issues than this one (also many years old), and only maybe one or two had longer list of "mentioned in another issue" activity as in this issue, clearly showing there are many more desperate developers experiencing this issue. In combination with the age of this issue, I'm wondering why this issue is labeled as "not interesting" by having P2 priority.

But I don't want to transgress the guidelines any further, so I'll cease this discussion and I'm sorry for starting it. I hope the Flutter team will reconsider the severity of this issue.

chipweinberger commented 7 months ago

Do plugins need to know about hot reload and hot restart?

Yes, ideally.

In flutter_blue_plus we disconnect all BLE devices after a hot restart. (in order to keep Dart state & native state in sync).

https://github.com/boskokg/flutter_blue_plus/blob/c5fa0701a7e5756f2328adea1ec606ce0d0aa4b7/android/src/main/java/com/lib/flutter_blue_plus/FlutterBluePlusPlugin.java#L297

If we knew the difference between Hot Restart vs App Launch, we could avoid a small amount of work, and some confusing log messages.

Currently we just assume it was a hot restart everytime Dart is initialized, and execute our hot restart code. It works.

av4625 commented 1 month ago

This would be great for my use case. I am using FFI to load in a shared library and it kicks off a long running thread when the flutter app starts and then in AppLifecycleListener.onExitRequested I stop that thread. But on hot restart, the thread doesn't stop and it tries to start a new one and "bad things happen". I have no real option but to quit and re run the app every time, which isn't ideal when hot restart is a feature.

muzzah commented 3 weeks ago

This is currently causing me issue with the firebase database plugin. When doing a hot restart the connection is not destroyed so any onDisconnect triggers are not fired even though previously it would. Im not sure why there is such a back and forward debate here about if this is useful. Its clear that developers are asking for it. Its clear its needed and useful, even for app developers who need to know a hot restart has happened and reinitialise connections or other state.

flutter / flutter

Do plugins need to know about hot reload and hot restart? #10437