Linux shell/embedder do not shutdown Dart root isolate when exiting

ndusart commented 11 months ago

Is there an existing issue for this?

[X] I have searched the existing issues
[X] I have read the guide to filing a bug

Steps to reproduce

Create base project with flutter create --platform=linux
Run the app
Close the application by closing button of the window

(This works also when calling SystemNavigator.pop()

Expected results

Window is properly detached from flutter view and root isolate is properly shut down.

Actual results

GTK runloop (g_application_run in main.cc of standard linux embedder of application) returns directly and the process exits immediately without the window to be properly detached.

Sometimes, this causes an abort before process exit and this is considered as an abnormal exit of the program.

Code sample

This is reproducible using template project created with flutter create.

Screenshots or Video

No response

Logs

No response

Flutter Doctor output

[✓] Flutter (Channel stable, 3.10.6, on Arch Linux 6.4.8-arch1-1, locale en_US.UTF-8) • Flutter version 3.10.6 on channel stable at /opt/flutter • Upstream repository https://github.com/flutter/flutter.git • Framework revision f468f3366c (4 weeks ago), 2023-07-12 15:19:05 -0700 • Engine revision cdbeda788a • Dart version 3.0.6 • DevTools version 2.23.1

[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.2) • Android SDK at /opt/android-sdk/ • Platform android-33, build-tools 33.0.2 • ANDROID_HOME = /opt/android-sdk • Java binary at: /opt/android-studio/jbr/bin/java • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) • All Android licenses accepted.

[✓] Chrome - develop for the web • CHROME_EXECUTABLE = chromium

[✓] Linux toolchain - develop for Linux desktop • clang version 15.0.7 • cmake version 3.27.1 • ninja version 1.11.1 • pkg-config version 1.8.1

[✓] Android Studio (version 2022.2) • Android Studio at /opt/android-studio • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694)

[✓] Connected device (3 available) • Pixel 7 (mobile) • 29261FDH2000H5 • android-arm64 • Android 13 (API 33) • Linux (desktop) • linux • linux-x64 • Arch Linux 6.4.8-arch1-1 • Chrome (web) • chrome • web-javascript • Chromium 115.0.5790.170 Arch Linux

[✓] Network resources • All expected network resources are available.

• No issues found!

huycozy commented 11 months ago

Could you be more specific on the Actual result? Is there an example that demonstrates abnormal exit more intuitively?

ndusart commented 11 months ago

The Dart VM is not properly shutdown during app normal exit. (this is not an abnormal exit condition here) You can verify it in the engine code by monitoring that root_isolate_shutdown_callback is not executed.

I managed to detect that on my custom branch

The issue here is that Dart VM do not properly clean its resources and one consequence of this is that NativeFinalizer are not collected and do not call its callback, while it is a strong guarantee of this.

The following example can be run in order to see that NativeFinalizer callback is not called when exiting application using window close button or calling SystemNavigator.pop().

Example project

**main.dart**: ``` import 'dart:ffi' as ffi; import 'dart:io'; import 'package:ffi/ffi.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; class libc { static final ffi.DynamicLibrary _lib = ffi.DynamicLibrary.process(); static final printf = _lib.lookup)>>("printf"); } void main() { runApp(const MyApp()); } class MyApp extends StatelessWidget { const MyApp({super.key}); @override Widget build(BuildContext context) { return MaterialApp( title: 'Flutter Demo', theme: ThemeData( colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple), useMaterial3: true, ), home: MyHomePage(title: 'Flutter Demo Home Page'), ); } } class MyHomePage extends StatefulWidget { MyHomePage({super.key, required this.title}) {} final String title; @override State createState() => _MyHomePageState(); } class _MyHomePageState extends State implements ffi.Finalizable { static final _finalizer = ffi.NativeFinalizer(libc.printf.cast()); @override void initState() { super.initState(); final str = "Native Finalizer called!\n".toNativeUtf8(); _finalizer.attach(this, str.cast()); } @override void dispose() { super.dispose(); } @override Widget build(BuildContext context) { return Scaffold( appBar: AppBar( backgroundColor: Theme.of(context).colorScheme.inversePrimary, title: Text(widget.title), ), body: Center( child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ const Text( 'Push button to exit app', ), ElevatedButton( child: Text("Exit"), onPressed: (){ SystemNavigator.pop(); }, ), ], ), ), ); } } ```

mraleph commented 11 months ago

(Removing dependency: dart because it is purely engine / embedding fix)

ndusart commented 11 months ago

hello @mraleph, thanks for the heads up.

Could you confirm this sounds to be due to how flutter is interacting with the Dart VM more than an issue in dart ?

I am discussing on a Discord thread and they imply I should probably move that ticket to dart directly.

I am not conviced this is related to dart but indeed is a defect in how flutter manage the dart vm. Would you like to share your opinion on this ?

Thanks :)

gspencergoog commented 11 months ago

As we discussed on the Discord thread, our philosophy on this issue is that providing a notification that isn't reliable gives a false sense of security to app developers that we don't want to leave them with. (Desktop Triage)

In your case, are you asking for a plugin API that notifies about the application exit, or a Framework notification through the AppLifecycleListener or bindings?

For posterity (in case the Discord record disappears), here's part of the OP's problem statement:

Hello Thanks again for discussing about this, that's appreciated 🙂 Maybe I
should continue on the github thread instead of here. Please let me know.

I think we can agree (partly for different reasons) on the fact that my solution
to actually wait for the event loop to be empty is effectively not feasible. The
main reason would be that it would transparently block lot of apps when they are
exiting because most flutter packages are not written with this in mind. [3:49
PM]ndusart: I think that you agree on the fact that releasing resources is
indeed important and expected during app exit. I'm quoting the doc of
ServicesBinding.exitApplication:

This differs from calling dart:io's exit function in that it gives the engine a
chance to clean up resources so that it doesn't crash on exit

So clean up resources is something you expect too. I may have read some of your
comments as it isn't so that's why I was arguing on this again. But if I
understand it right now, I'll stop arguing on this and focus on how to provide
this facility. If I am correct, the difference of opinion is more about that in
current state, app developers are able to correctly clean their resources.
Please let me know if I misread something. [3:49 PM]ndusart: Here are some
resources that can be found difficult to properly clean in the current
situation: [3:50 PM]ndusart: We wrote a Flutter plugin to read smart cards. It
uses the USB Android Sdk, ExternalAccessory SDK on iOS and PC/SC on desktops.
When application is shutting down, we have to release the smart card readers if
it is currently in use. We use native code so we have to use Platform Channels
which are inherently asynchronous. On mobile, we could use
didChangeAppLifecycleState to listen for the "detached" state. But it is
synchronous, so we can't ensure our native code has effectively finished
operation. On desktop, didRequestAppExit (like you mentioned) is not usable
because application may be exited through the close button of the window (or
with SIGINT/SIGTERM signals which should be considered as normal exit request
because that's what they are, SIGKILL is the force termination) and this is not
calling this callback and it is currently not catchable without messing with the
linux embedder code. You could argue that this would be cleaned by the OS but
not really. The reader would be released but the smart card may be left in an
unknown state which can be problematic if left in an unlocked state. Also, we
run this on some proprietary Android solutions with their own smart card
hardware and drivers, so we cannot rely on Android SDK for these. For some of
these platforms, if you don't actually release the resource, it won't be
automatically released in case of normal exit. Because, as mentioned several
times, on Android, the process is kept alive, so the driver won't see that the
file descriptor used to communicate with him has been closed. Ironically, device
power loss is actually preferrable than normal exit, because in this situation
we can be sure that the smart card in the reader is not powered anymore and is
reset. Of course our app is coded to handle restart after a crash and that smart
card may be in an unknown state, that is problematic because this may be abused
by malicious processes and this condition is now occuring on every app exit.
[3:50 PM]ndusart: In this same application, we want to provide a feature to
recover data of last opened document if user closed the app by inattention. So,
this effectively write the document on a temp file, flush the buffers and
atomically rename it to the current document "working" file. This again can be
handled on mobile with didChangeAppLifecycleState and synchronous is less
problematic because File offer synchronous operations. But we lose the
possibility to actually do this file save operation while releasing the smart
card reader talked in the previous point (user could be in the operation to sign
it using his smart card) as file I/O shouldn't prevent us to do this
concurrently. And if speaking about OS auto clean, on Android, this won't be
cleaned automatically by the OS, as mentioned, the process is not exited, it is
handled by the Flutter engine actually. The same problem applies to Linux as the
previous point. I gave an example
[here](https://github.com/flutter/flutter/issues/40940#issuecomment-1658736915)
where a TCP port (obtained using native syscalls) is kept opened on Android and
app is not able to restart correctly without being killed. That's related to my
previous point where it may be misleading to think that all resources will be
released because most of I/O resources are actually managed by the Flutter
engine with creating these using Dart language only. In this case, Android
tendency to keep process alive prevent this resource to be reclaimed by the OS
and we are forced to use didChangeAppLifecycleState. [3:50 PM]ndusart: Some
protocols above TCP may expect a proper close message (like WebSocket) to handle
connection closed by peer in a timely manner. Failing to do so may consume more
resource for the other peer that will treat the TCP socket close as an abnormal
condition. Another person [mentioned
this](https://github.com/flutter/flutter/issues/40940#issuecomment-982460866):
when using DNS-SD, if you failed to send an unregister message, your service is
still registered and clients may still try to connect to you until some other
form of failure detection triggers. A graceful unregistration during app exit
would have save resources on plenty of other devices. [3:50 PM]ndusart: Also
recovery from crashed condition may have a runtime cost that is not neglectible,
especially when app startup time can be an important matter. This recovery
mechanism is likely to be triggered during restart after each app exit.

I still think that a way to execute code just before dart VM is shut down is
still important. Your proposition about a new callback in WidgetsBindingObserver
seem to be a proper candidate in my mind. It would let app developers optionnaly
use it and it won't impact the whole community and force people to adapt their
application. But I do think that this callback should be async and be awaited
before letting the dart VM shutdown

ndusart commented 10 months ago

In your case, are you asking for a plugin API that notifies about the application exit, or a Framework notification through the AppLifecycleListener or bindings?

This issue is more generic about how the close of the application can abruptly shutdown the dart VM without any notice.

This can be visualised more specifically by seeing how SystemNavigator.pop() very often crashes the process on Linux. There is clearly a race condition where given the timing, this may just exit the process without causing any problem but it is very easy to create an abort condition in a simple basic project only calling SystemNavigator.pop() in a push of a button.

This let met think that the engine or the embedder may need to properly handle the process exit to cleanly stop threads and release some resources. If you think you should fix SystemNavigator.pop() to do something else than basically calling exit(0) (which it does now) to let the flutter engine code to close properly, I'd be interested to know the reasons why letting handle some (not all, just the same as the ones handled by the engine) exit condition is not applicable to app code as well.

The initial point of this specific issue was that imho at very least, Dart VM should be properly shutdown. This would let plugin developers rely on NativeFinalizer to be executed on normal exit conditions.

You seem to disagree but actually exit condition that are not catchable already have the expected behavior for app developers, we do not need more guarantees, because in all platforms, an uncatchable exit condition express itself as a crash condition or clear stop of any code execution (e.g. power loss) and in the case the system is still running, OS will recover most if not all resources. This is expected and developers can code around these facts.

The problematic case in Flutter environment are normal exit condition, which are effectively catchable and reliable. In these conditions, either:

we end up with a running process on Android and OS is not reclaiming resources. This leads to stall resources that cannot be reclaimed and leaks.
in case where process exits (on desktop environments), for some resources we cannot avoid a crash condition while the process should exit cleanly

Exit notification is somewhat helping. Since 3.13, Linux is better served because we can at least now intercept the click on the close button of the main window by listening for didRequestAppExit callback.

But, consider app developers depending on some native plugins (they probably even don't know the fact that they are native). They now have to know that they should explicitely close an object/controller/handle from a native plugin to release some unknown resources because this plugin cannot rely on NativeFinalizer to fire under normal exit conditions in all platforms. This close operation should be also called in multiple callback notifying for exit condition because every platform has a different mechanism.

That's highly unexpected and that's causing confusion and issues here and there.

From my experience, mobile platforms are properly closing the dart VM and we can rely on NativeFinalizer on these. Cannot speak about other desktop platforms, but adding this behavior on Linux would help unify how we can manage native resources. And moreover, this could even be transparent to plugin users.

flutter-triage-bot[bot] commented 1 month ago

The triaged-desktop label is irrelevant if there is no team-desktop label or fyi-desktop label.

flutter / flutter