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

Activity

1. huycozy

   added

   in triagePresently being triaged by the triage teamPresently being triaged by the triage team

   on Aug 14, 2023
2. huycozy commented on Aug 14, 2023

   

   huycozy

   on Aug 14, 2023

   Member

   More actions

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

   added

   waiting for responseThe Flutter team cannot make further progress on this issue until the original reporter respondsThe Flutter team cannot make further progress on this issue until the original reporter responds

   on Aug 14, 2023
4. ndusart commented on Aug 21, 2023

   

   ndusart

   on Aug 21, 2023

   Last edited by ndusart

   Author

   More actions

   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<ffi.NativeFunction<ffi.Void Function(ffi.Pointer<Utf8>)>>("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<MyHomePage> createState() => _MyHomePageState();
   }
   
   class _MyHomePageState extends State<MyHomePage> 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: <Widget>[
               const Text(
                 'Push button to exit app',
               ),
               ElevatedButton(
                 child: Text("Exit"),
                 onPressed: (){
                   SystemNavigator.pop();
                 },
               ),
             ],
           ),
         ),
       );
     }
   }
   
5. github-actions

   removed

   waiting for responseThe Flutter team cannot make further progress on this issue until the original reporter respondsThe Flutter team cannot make further progress on this issue until the original reporter responds

   on Aug 21, 2023
6. huycozy

   added

   engineflutter/engine related. See also e: labels.flutter/engine related. See also e: labels.

   platform-linuxBuilding on or for Linux specificallyBuilding on or for Linux specifically

   a: desktopRunning on desktopRunning on desktop

   team-desktop

   dependency: dartDart team may need to help usDart team may need to help us

   and removed

   in triagePresently being triaged by the triage teamPresently being triaged by the triage team

   on Aug 22, 2023
7. mraleph

   removed

   dependency: dartDart team may need to help usDart team may need to help us

   on Aug 22, 2023
8. mraleph commented on Aug 22, 2023

   

   mraleph

   on Aug 22, 2023

   Member

   More actions

   (Removing dependency: dart because it is purely engine / embedding fix)
9. ndusart commented on Aug 22, 2023

   

   ndusart

   on Aug 22, 2023

   Author

   More actions

   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 :)
10. gspencergoog commented on Aug 24, 2023

    

    gspencergoog

    on Aug 24, 2023

    Last edited by gspencergoog

    Contributor

    More actions

    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
    
11. gspencergoog

    added

    P3Issues that are less important to the Flutter projectIssues that are less important to the Flutter project

    triaged-desktop

    on Aug 24, 2023
12. ndusart commented on Aug 31, 2023

    

    ndusart

    on Aug 31, 2023

    Last edited by ndusart

    Author

    More actions

    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.

    👍React with 👍1Reacted by yvs2014
13. cbracken

    added

    team-linuxOwned by the Linux platform teamOwned by the Linux platform team

    and removed

    team-desktop

    on Jun 6, 2024
14. flutter-triage-bot

    removed

    triaged-desktop

    on Jun 6, 2024
15. flutter-triage-bot commented on Jun 6, 2024

    

    flutter-triage-botbot

    on Jun 6, 2024

    More actions

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

    added

    triaged-linuxTriaged by the Linux platform teamTriaged by the Linux platform team

    on Jun 7, 2024