Skip to content

Defer execution of main until resume for hot restart with DDC library bundle format #2623

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 9, 2025

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented May 23, 2025

pause_isolates_on_start tells DWDS and the client that during a hot restart or a hot reload, the VM service will pause and is actively waiting for the client to remove existing breakpoints, reregister them, and then resume. It lets the client know by sending a PausePostRequest event.

In order to do this in DWDS, we need to defer the execution of main until that resume. So, like we do with the require restarter, we wait for a completer to finish before we call main after a hot restart. This completer is only provided when the flag is enabled.

Fixes existing code that marks the completer as completed before running main. The previous code canceled the subscription in an unawaited Future. This may result in us recalling main because the event stream could still have a listener. An example is if we hit a breakpoint immediately after main and call resume.

Also fixes an issue where metadata information isn't recomputed on a hot restart. This is needed when new files are added.

Adds tests for:

  • Modifying a line with a breakpoint and restarting.
  • Adding a line before a breakpoint and restarting.
  • Removing a line before a breakpoint and restarting.
  • Adding a file and putting a breakpoint in it before restarting.

… bundle format

pause_isolates_on_start tells DWDS and the client that during a
hot restart or a hot reload, the VM service will pause and is
actively waiting for the client to remove existing breakpoints,
reregister them, and then resume. It lets the client know by
sending a kPausePostRequest event.

In order to do this in DWDS, we need to defer the execution of
main until that resume. So, like we do with the require
restarter, we wait for a completer to finish before we call
main after a hot restart. This completer is only provided when
the flag is enabled.

Fixes existing code that marks the completer as completed before
running main. The previous code canceled the subscription in an
unawaited Future. This may result in us recalling main because
the event stream could still have a listener. An example is if
we hit a breakpoint immediately after main and call resume.

Also fixes an issue where metadata information isn't recomputed
on a hot restart. This is needed when new files are added.

Adds tests for:

- Modifying a line with a breakpoint and restarting.
- Adding a line before a breakpoint and restarting.
- Removing a line before a breakpoint and restarting.
- Adding a file and putting a breakpoint in it before restarting.
@srujzs srujzs requested review from nshahan and bkonyi May 23, 2025 04:43
@srujzs
Copy link
Contributor Author

srujzs commented May 27, 2025

I believe this should be ready for review - the test failures seem to be tied to existing failures which require investigation.

@srujzs
Copy link
Contributor Author

srujzs commented May 28, 2025

@bkonyi Whenever you get a chance, can I have you review this as well?

Copy link
Collaborator

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

LGTM overall. It might be worth adding some logic to the tests to verify that the isolates no longer have breakpoints set in libraries that were reloaded or that all breakpoints have been removed after a restart.

…ns of console logs after resume to use a future instead
@srujzs
Copy link
Contributor Author

srujzs commented May 30, 2025

Thanks! I added logic to check that there are no breakpoints in the isolate after a hot restart but before we process a PausePostRequest to register breakpoints again.

srujzs added 2 commits May 30, 2025 17:48
- Recompile is needed for the frontend server even when no
edits are made
- Also use expectAll in hot_restart_breakpoints_test
@srujzs srujzs merged commit 661dafd into dart-lang:main Jun 9, 2025
47 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jun 9, 2025
Revisions updated by `dart tools/rev_sdk_deps.dart`.

ai (https://github.com/dart-lang/ai/compare/1d9d60c..f2b48c6):
  f2b48c6  2025-06-09  Nate Bosch  Retain compatibility with 3.7 SDKs (dart-lang/ai#163)
  52adf08  2025-06-06  Jacob MacDonald  add homepage, repository, and documentation links to the pub result (dart-lang/ai#155)
  fa1c2be  2025-06-06  Nate Bosch  Always use the failures-only reporter for testing (dart-lang/ai#160)
  66a152f  2025-06-06  Nate Bosch  Instruct clients to prefer MCP (dart-lang/ai#161)
  55ad850  2025-06-05  Jacob MacDonald  Add a retroactive changelog (dart-lang/ai#157)
  b08a610  2025-06-05  Jacob MacDonald  Update instructions, add cursor install link (dart-lang/ai#159)

web (https://github.com/dart-lang/web/compare/f1becf0..de6b3e4):
  de6b3e4  2025-06-06  Srujan Gaddam  Add missing copyrights and delete empty files (dart-lang/web#371)
  74a33ba  2025-06-06  Kevin Moore  Add in a missing library directive, missing new line (dart-lang/web#370)
  4d24eb5  2025-06-06  nikeokoronkwo  [web_generator] Setting up `web_generator` for Dart JS Interop Gen (dart-lang/web#368)

webdev (https://github.com/dart-lang/webdev/compare/55941b0..661dafd):
  661dafd4  2025-06-08  Srujan Gaddam  Defer execution of main until resume for hot restart with DDC library bundle format (dart-lang/webdev#2623)
  01a3b9d7  2025-06-06  Nicholas Shahan  Remove skip from chrome proxy service test

Change-Id: If3d4326d0bacf47a4d95520dbd0aac5fb58de439
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/433363
Commit-Queue: Devon Carew <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants