[flutter_tools] add preHotRestart callbacks by jonahwilliams · Pull Request #86134 · flutter/flutter

jonahwilliams · 2021-07-08T23:43:24Z

Adds a new API to register a callback that will be invoked + awaited before a hot restart is initiated:

class Foo {
  Foo() {
    var data = acquireNativeResource();
   if (kIsDebugMode) {
     WidgetsBinding.instance!.registerHotRestartCallback(data.release, debugLabel: 'Foo Native Resource Release');
   }
  }
}

With this I was able to wire up the folloing to allow me to hot restart my little roguelike without crashing:

class SoundSystemImpl implements SoundSystem {
  SoundSystemImpl() {
    _init();
  }

  final _wavs = <String, Pointer<Wav>>{};
  Point _listener = Point.zero;

  late final Pointer<Soloud> _audioContext;

  void _init() {
    if (!kSoundEnabled) {
      return;
    }
    _audioContext = _soloudCreate();
    _soloudInitEx(
        _audioContext,
        SOLOUD_CLIP_ROUNDOFF | SOLOUD_ENABLE_VISUALIZATION,
        SOLOUD_AUTO,
        SOLOUD_AUTO,
        SOLOUD_AUTO,
        SOLOUD_AUTO);
    _soloudSetGlobalVolume(_audioContext, 2);
    WidgetsBinding.instance!.registerHotRestartCallback(_destroy, debugLabel: 'Soloud'); // NEW
  }

  void _destroy() {
    _soloudDeInit(_audioContext);
    _soloudDestroy(_audioContext);
  }

…nto hot_restart_hook

jonahwilliams · 2021-07-09T00:48:04Z

+  ///
+  /// This can be used to release native resources acquired through platform
+  /// channels or `dart:ffi`.
+  void registerHotRestartCallback(PreHotRestartCallback callback, {String? debugLabel}) {


I could give a much larger example of how to use this based on some dart:ffi native code (since I have an implementation that uses this sitting right here...)

the more documentation the better!

done-ish (could add the native code example too)

jonahwilliams · 2021-07-09T00:49:32Z

 export 'dart:ui' show AppLifecycleState, Locale;

+/// A callback that can be registered with [WidgetsBinding.registerHotRestartCallback].
+typedef PreHotRestartCallback = FutureOr<void> Function();


I don't usually like FutureOr, but in this case since we want to support async functions, the futureOr lets you pass tear-offs of void returning methods without wrapping in another function.

var foos = <int>[]; ... WidgetsBinding.instance!.registerHotRestartCallback(foos.clear); // vs WidgetsBinding.instance!.registerHotRestartCallback(() async => foos.clear());

Future<void>? is probably better. Or just Future<void> since really it's trivial to add async to a function.

Object? will also accept either a future returning function or a void function/tear-off, i'll switch to that

Hixie · 2021-07-09T01:52:39Z

+  ///
+  /// This can be used to release native resources acquired through platform
+  /// channels or `dart:ffi`.
+  void registerHotRestartCallback(PreHotRestartCallback callback, {String? debugLabel}) {


you can simplify some of the code here by making debugLabel have a default value ("unknown" or something)

Hixie · 2021-07-09T01:55:30Z

+
+        await Future.wait(<Future<void>>[
+          for (MapEntry<PreHotRestartCallback, String?> entry in _hotRestartCallbacks.entries)
+            invokeAndWait(entry.key, entry.value).timeout(const Duration(seconds: 5), onTimeout: () {


If we have timeouts here, we really should do them on the tool side... but I would just remove the timeouts entirely.

Optionally, we could make a second hot restart request while one is already ongoing just force a hot restart without triggering these callbacks.

I can remove the timeouts here, possibly use the VM service notification API to let the tool know which callbacks are pending. then the timeout warning could include what exactly is stuck.

Rather than a second hot restart, we could prompt to continue ignoring the callbacks as long as a terminal is attached. If one of these is caught in an infinite loop though, there isn't anything we can do (though that is true of other parts of the framework too: #85987)

Prompting requires more work to make this work in IDEs. Forcing the hot restart is always what you're going to want, as a developer. When would you ever say "no, I'd rather not do the thing I asked for actually, just continue waiting on this thing that's never going to finish"? :-)

Hixie · 2021-07-09T01:56:46Z

      // printHotReloadWorked();

-      print('((((TICK $count))))');
+      print('((((TICK \$count))))');


oh i see, wow. how did that not get caught before.

what is count even resolving to here?!

oh, this was previously a raw string so that was safe, but I want to conditionally embed some code so I switched it out to a regular string and escaped the count interpolation to keep the same behavior.

Hixie · 2021-07-09T01:58:34Z

+          try {
+            await callback();
+          } catch (error) {
+            debugPrint('Failed to invoke preHotRestartCallback ${label ?? 'unlabeled'}: $error');


we have logic for this (FlutterError and so forth). If for some reason we can't use it here it would be good to leave a comment saying why.

Hixie · 2021-07-09T01:59:00Z

+        await Future.wait(<Future<void>>[
+          for (MapEntry<PreHotRestartCallback, String?> entry in _hotRestartCallbacks.entries)
+            invokeAndWait(entry.key, entry.value).timeout(const Duration(seconds: 5), onTimeout: () {
+              debugPrint('preHotRestartCallback ${entry.value ?? 'unlabeled'} is taking longer than expected...');


i'd probably put the name in quotes

Hixie · 2021-07-09T02:11:17Z

(re the customer_testing-linux failure, #86143 to add more debugging)

zanderso · 2021-07-09T15:33:49Z

@chaselatta @arbreng @jxson This PR provides a hook that could also be useful for getting Hot Restart to work on Fuchsia, for example for tearing down handles that would otherwise leak and/or whose initialization is not idempotent.

…hook

jonahwilliams · 2021-07-09T19:04:09Z

Still working on changing how timeouts work

…nto hot_restart_hook

jonahwilliams · 2021-07-13T21:56:43Z

This is ready for another review

zanderso

tool changes lgtm

…hook

jonahwilliams · 2021-07-22T16:06:29Z

Friendly ping @Hixie

Hixie · 2021-07-22T18:56:16Z

+  ///   /// be re-acquired.
+  ///   late final Pointer _context = _createContext();
+  /// }
+  /// ```


do you know if this code is getting analyzed?

In its current form, this is most likely not analyzed. It should be wrapped in a {@tool snippet ...} or {@tool sample ...} section, so the code here doesn't become stale.

Hixie · 2021-07-22T18:58:26Z

 export 'dart:ui' show AppLifecycleState, Locale;

+/// A callback that can be registered with [WidgetsBinding.registerHotRestartCallback].
+typedef PreHotRestartCallback = Object? Function();


This should document what the return value should be. It's very unusual for an API to just allow any return value.

(I don't understand why this can't just be void or Future<void> though?)

Hixie · 2021-07-22T18:59:58Z

+              )
+            );
+          } finally {
+            developer.postEvent('preHotRestartCallback', <String, Object>{'label': label, 'finished': true});


why postEvent/postEvent rather than Timeline.startSync/finishSync?

(I don't see this event being used anywhere?)

Hixie · 2021-07-22T19:01:23Z

+  /// tearing down a native resource acquired through `dart:ffi`. In this example, if
+  /// the `context` pointer is not passed through to the `_destroyContext` function before
+  /// a hot restart, the application will crash after a hot restart.
+  ///


these docs should say what happens if you register the same callback twice

Hixie · 2021-07-22T19:02:00Z

+  FlutterRunTestDriver flutter;
+
+  setUp(() async {
+    tempDir = createResolvedTempDirectorySync('hot_reload_test.');


style guide says temp directories must start with flutter_

Hixie · 2021-07-22T19:06:06Z

cc @goderbauer since this is core framework stuff (feel free to land if you've addressed my comments and @goderbauer gives you an LGTM at some future point).

goderbauer · 2021-07-26T16:53:20Z

+  ///   /// be re-acquired.
+  ///   late final Pointer _context = _createContext();
+  /// }
+  /// ```


In its current form, this is most likely not analyzed. It should be wrapped in a {@tool snippet ...} or {@tool sample ...} section, so the code here doesn't become stale.

goderbauer · 2021-07-26T16:56:39Z

+    if (!kDebugMode) {
+      return;
+    }
+    _hotRestartCallbacks[callback] = debugLabel;


Maybe document that registering the same callback multiple times will only invoke it once (which differs from callback behavior we have in many other places of the framework).

Maybe also document that the callback is persistent?

goderbauer · 2021-07-26T16:59:34Z

+  /// class NativeResourceService {
+  ///   NativeResourceService() {
+  ///     if (kDebugMode) {
+  ///       WidgetsBinding.instance!.registerHotRestartCallback(() => _destroyContext(_context), debugLabel: 'NativeResourceService');


How do I unregister this callback if I no longer need it? (e.g. because I've manually disposed this service as it's no longer needed for the functionality of my app)

christopherfujino · 2021-08-12T20:54:26Z

@jonahwilliams are you still working on this one?

jonahwilliams · 2021-08-12T22:14:13Z

I'm not, closing

[flutter_tools] add preHotRestart callbacks

f2bc8c2

flutter-dashboard Bot added framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Jul 8, 2021

google-cla Bot added the cla: yes label Jul 8, 2021

Jonah Williams and others added 3 commits July 8, 2021 17:04

Update pre_hot_restart_callback_test.dart

e12e165

expand test cases

88307be

Merge branch 'hot_restart_hook' of github.com:jonahwilliams/flutter i…

6b03ebe

…nto hot_restart_hook

jonahwilliams marked this pull request as ready for review July 9, 2021 00:47

jonahwilliams requested review from Hixie and zanderso July 9, 2021 00:47

jonahwilliams commented Jul 9, 2021

View reviewed changes

Hixie reviewed Jul 9, 2021

View reviewed changes

jonahwilliams added 2 commits July 9, 2021 10:52

Merge branch 'master' of github.com:flutter/flutter into hot_restart_…

100ce59

…hook

address some code review

b5b2260

Jonah Williams and others added 4 commits July 12, 2021 09:04

Merge branch 'master' into hot_restart_hook

9c3504f

fix unit tests

9e582f2

Merge branch 'hot_restart_hook' of github.com:jonahwilliams/flutter i…

ef657d2

…nto hot_restart_hook

add timeout warning on tools side

aa5693d

jonahwilliams requested a review from Hixie July 12, 2021 18:10

zanderso approved these changes Jul 15, 2021

View reviewed changes

Merge branch 'master' of github.com:flutter/flutter into hot_restart_…

2620737

…hook

Hixie reviewed Jul 22, 2021

View reviewed changes

goderbauer reviewed Jul 26, 2021

View reviewed changes

christopherfujino added waiting for tree to go green waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds and removed waiting for tree to go green labels Aug 12, 2021

jonahwilliams closed this Aug 12, 2021

jonahwilliams deleted the hot_restart_hook branch August 12, 2021 22:14

jonahwilliams restored the hot_restart_hook branch September 5, 2021 19:22

maks mentioned this pull request Nov 8, 2022

Add hot restart hooks to support dart:ffi #75528

Open

davidmartos96 mentioned this pull request Jul 15, 2023

[flutter_tools] add preHotRestart callbacks #130662

Closed

8 tasks done

Conversation

jonahwilliams commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 9, 2021

Uh oh!

zanderso commented Jul 9, 2021

Uh oh!

jonahwilliams commented Jul 9, 2021

Uh oh!

jonahwilliams commented Jul 13, 2021

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jul 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jonahwilliams commented Jul 8, 2021 •

edited

Loading

jonahwilliams Jul 9, 2021 •

edited

Loading