Repository navigation
[flutter_tools] add preHotRestart callbacks - #130662
davidmartos96 wants to merge 51 commits into
Conversation
…nto hot_restart_hook
…nto hot_restart_hook
github-actions
Bot
added
tool
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
Just some fly-by comments during triage.
Thanks for the comments! |
|
This is awesome! Please prioritize getting this over the finish line! |
|
Friendly ping @christopherfujino |
|
Will take a look at this the week after next when @goderbauer is back in the office. |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@christopherfujino Any update? CI is green now, the number of extensions in the test needed to be updated after merging with master |
@davidmartos96 I apologize for the long delay getting back to you on this PR. In real usage of this app, would it not be possible that the app fail without closing the connection, and then restart with the same client ID, leading to this same issue? In other words, how is the hot restart use case different from the app getting terminated and restarted? |
|
@christopherfujino On Flutter Web, when restarting the page using the browser toolbar (F5), the websocket connection is closed. The problem is that hot restart (on web) doesn't behave the same way, and it stays open. If the app fails, Flutter provides ways to deal with global errors, so you could close connections if you need to from the error handler. |
Ahh ok, I didn't realize you were targetting flutter for web. I was imagining a mobile app.
You're talking here about thrown Dart errors, right? I was imagining a failure where the VM immediately died. However, in the context of the web maybe the browser handles cleaning up the websocket... |
|
@davidmartos96 I'm sorry for the looong delay on this PR, and that I'm going to be closing it. This was a difficult decision because I agree that this is a legitimate bug in the Flutter SDK and conceptually this seems like a sound solution. The honest truth though is that I do not understand well enough all of the implications of this change, and whether or not the behavior would be correct for all flutter developers. Moving forward, I filed #144737 for the web use case. There are a set of known issues with hot restart for the web, and I'm hopeful we can have a more general solution. For the situation with dart:ffi, I think the right solution here would be for Dart finalizers to run on hot restart, which is tracked at dart-lang/sdk#50016. Again I apologize for the way code review of this PR was handled, and I do appreciate your contribution. All the best. |
|
@christopherfujino Thank you. I can understand the reason. Hot restart is a core feature in the framework which needs to be handled with care. The original motivation of the PR was to have a way to close socket connections. If there is an automatic way to do that it would be great. With this approach the user would need to explicitly call close, whereas in This PR was a nice chance to dig into the framework and see how the restart features are wired together, no work is lost 🙂 |
AFAIK there is no out of the box automated way to do this. I can't promise anything, but if you follow #144737 I will make sure it stays up to date. There are other related problems, and work going on that--if it lands--would give you the hot restart semantics you want. I can't promise because, as with this PR, the implications are deep, and it's still unclear the full impact of changing the semantics.
I'm glad, and again I appreciate your contribution to a particularly tricky part of the codebase. Hope to see you around in the future! |
…orHandling.handleError - Explain in `FutureErrorHandling.handleError`'s doc comment why `Future.then` is used instead of `Future.catchError` or `Future.onError` (`AvoidFutureCatchError` custom analyzer rule in `dev/bots/custom_rules/avoid_future_catcherror.dart`, flutter#130662, dart-lang/sdk#51248).
8 participants
Fixes #75528
Work on top of @jonahwilliams PR #86134
Hello! I took a stab at this, by updating the code from Jonah with latest master.
On top of the previous code I also added support for Pre Hot Restart callbacks to the web runner.
I did some real usage tests with this and it is exactly what I needed. On my project, a websocket connection is stablished when the app runs. Whenever the app does a hot restart, there is not a chance for the app to close it, so the socket server thinks another connection is made with the same "client ID" which is unexpected in our app domain. This hook provides the chance to close the connection and then restart normally.
I'm sure there are other use cases where this functionality could be really handy like native handles from FFI showcased in the original issue or Firestore streams as manifested by @maks here #75528 (comment)
@jonahwilliams What was exactly what you were thinking about reworking regarding the timeouts?
Do you remember if there was any other thing you were planning to change before stopping working in the PR?
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.