Skip to content
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

Wait for a resume event to run the main() method after a page refresh #2431

Merged
merged 3 commits into from
May 14, 2024

Conversation

elliette
Copy link
Contributor

@elliette elliette commented May 10, 2024

I was putting together a doc about the many places we wait for a resume event after either a hot-restart or a page-refresh when pauseIsolatesOnStart is true, and realized that I had missed waiting for the resume event for page refreshes when running an app with flutter_tools.

I then realized that the solution implemented for waiting for a resume event after a page-refresh for internal apps (#2398 and cl/626138927) could be modified to work for both flutter_tools and internal apps. These modifications are in this PR.

Before this change:

  • DWDS exposed a method shouldPauseIsolatesOnStart which could be called by a client (flutter_tools, DDR) to determine whether or not to immediately call DWDS' runMain method after an app connection event

With this change:

  • flutter_tools and DDR immediately call DWDS' runMain method after an app connection event
  • runMain waits for the _readyToRunMain Future before running the app's main() method
  • This _readyToRunMain Future is resolved once DWDS receives a resume event

Work towards flutter/devtools#7231

Requires cl/632610137 to be submitted as well

readyToRunMainCompleter,
);
} else {
// Otherwise, we can run the app's main() method immediately.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there specific configurations where we want this or is this just a backwards compatible fallback to what we did previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just backwards compatible fallback - all debugging clients should be setting the pauseIsolatesOnStart flag

_resumeAfterHotRestartEventsController.stream;
/// IMPORTANT: This should only be listened to during a hot-restart or page
/// refresh. The debugger ignores any resume events as long as there is a
/// subscriber to this stream.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean the debugger listens and cancels the listener after for every refresh event? I don't have any problem with that, I'm just asking if I understand this correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly - the stream is listened to only for the duration of the page refresh

@elliette elliette merged commit 99abc53 into dart-lang:master May 14, 2024
47 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request May 16, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

async (https://github.com/dart-lang/async/compare/09cba7f..77a25d7):
  77a25d7  2024-05-15  Sarah Zakarias  Add `topics` to `pubspec.yaml` (dart-lang/async#274)

dartdoc (https://github.com/dart-lang/dartdoc/compare/2e706be..476d5cc):
  476d5cc8  2024-05-16  Sam Rawlins  Refactor PackageGraph._tagReexportsFor and document (dart-lang/dartdoc#3772)
  24658cca  2024-05-14  Sam Rawlins  Privatize CommentReferenceParser._codeRef (dart-lang/dartdoc#3771)
  dff86ed9  2024-05-14  Sam Rawlins  Bump to 8.0.9 (dart-lang/dartdoc#3770)

http (https://github.com/dart-lang/http/compare/4722e03..76deb75):
  76deb75  2024-05-16  Hossein Yousefi  [cronet_http] Upgrade jni to 0.9.2 and publish 1.2.1 (dart-lang/http#1198)
  ec55561  2024-05-15  Brian Quinlan  [cronet] Use the same host and Android emulator architecture. (dart-lang/http#1201)

lints (https://github.com/dart-lang/lints/compare/f0205c1..b254c7e):
  b254c7e  2024-05-16  Devon Carew  Update README.md (dart-lang/lints#189)
  5fef508  2024-05-13  Lasse R.H. Nielsen  Tighten up the `gen_docs.dart` script. (dart-lang/lints#187)

test (https://github.com/dart-lang/test/compare/84d2a2b..2464ad5):
  2464ad5c  2024-05-16  Sarah Zakarias  Add `topics` to  package "test" `pubspec.yaml` (dart-lang/test#2230)
  6540a360  2024-05-15  dependabot[bot]  Bump the github-actions group across 1 directory with 3 updates (dart-lang/test#2229)
  4b6f029c  2024-05-15  dependabot[bot]  Bump dart_flutter_team_lints from 2.1.1 to ^3.0.0 in all packages (dart-lang/test#2228)

webdev (https://github.com/dart-lang/webdev/compare/d46cf50..fc32eb6):
  fc32eb69  2024-05-14  Elliott Brooks  Collect log message count and log at the end (dart-lang/webdev#2430)
  99abc535  2024-05-14  Elliott Brooks  Wait for a `resume` event to run the `main()` method after a page refresh (dart-lang/webdev#2431)

Change-Id: Iee28bacfc028c92e4b59d95f0c7e61f8282d2968
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366980
Auto-Submit: Devon Carew <[email protected]>
Commit-Queue: Konstantin Shcheglov <[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.

2 participants