Skip to content

[flutter_tools] alternate the name of the dill file used for hot restart#65435

Merged
jonahwilliams merged 2 commits intoflutter:masterfrom
jonahwilliams:toggle_main_dill_name
Sep 9, 2020
Merged

[flutter_tools] alternate the name of the dill file used for hot restart#65435
jonahwilliams merged 2 commits intoflutter:masterfrom
jonahwilliams:toggle_main_dill_name

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Sep 9, 2020

Description

The engine memory maps the dill file used to run the Dart isolate. Replacing this file before hot restart is not technically safe, though is unlikely to crash. Nevertheless, the protection applied by the windows mapping prevents replacing the file.

Update hot restart so that the uploaded file name alternates -> main.dart.dill -> main.dart.swap.dill > main.dart.dill to prevent replacing the mapped dill and to avoid filling up the devFS.

Context:

https://github.com/flutter/engine/blob/master/fml/platform/win/mapping_win.cc#L73

Fixes #60910

@flutter-dashboard flutter-dashboard bot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Sep 9, 2020
@jonahwilliams
Copy link
Contributor Author

FYI @aam

@jonahwilliams jonahwilliams marked this pull request as ready for review September 9, 2020 16:12
@jonahwilliams
Copy link
Contributor Author

@zanderso do you have any concerns about this? from my understanding what we're doing today is technically not safe, though unlikely to crash.

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bikeshed: instead of 'left' how about just '1' and '0'? Or maybe 'swap' or 'secondary'?

Is the real fix a lot more involved? I would think that Restart should be doing something like:

  1. stop execution
  2. unmap the old dill
  3. map the new dill
  4. restart

Do we know how/why that goes wrong?

@jonahwilliams
Copy link
Contributor Author

swap sounds good.

For a real fix, yes I think we'd have to stop the main isolate before uploading the new dill and restarting. I can foresee some issues though because the main isolate lifecycle is tied to the service isolate and application lifecycle, so simply using the existing vm_service API is not sufficient.

We'd need a new set of vm_service APIs provided by the engine that essentially split the hot restart process in half, one to take down the main isolate (while keeping everything else alive) and one to resurrect it with the newly uploaded dill. I'm not sure how difficult that would be to do. Fyi @chinmaygarde @jason-simmons

@jonahwilliams jonahwilliams merged commit 6d36056 into flutter:master Sep 9, 2020
@jonahwilliams jonahwilliams deleted the toggle_main_dill_name branch September 9, 2020 22:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows Hot Restart reverts to older version of app (from the first Hot Restart)

4 participants