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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dwds/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## 24.1.0-wip

- Fix bug where debugging clients are not aware of service extensions when connecting to a new web app. - [#2388](https://github.com/dart-lang/webdev/pull/2388)
- Respect the value of `pause_isolates_on_start` during page-refreshes. - [#2431](https://github.com/dart-lang/webdev/pull/2431)

## 24.0.0

Expand Down
3 changes: 0 additions & 3 deletions dwds/lib/dart_web_debug_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,4 @@ class Dwds {
debugSettings.enableDebugging,
);
}

bool shouldPauseIsolatesOnStart(String appId) =>
_devHandler.shouldPauseIsolatesOnStart(appId);
}
9 changes: 8 additions & 1 deletion dwds/lib/src/connections/app_connection.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ class AppConnection {
final _startedCompleter = Completer<void>();
final _doneCompleter = Completer<void>();
final SocketConnection _connection;
final Future<void> _readyToRunMain;

AppConnection(this.request, this._connection) {
AppConnection(this.request, this._connection, this._readyToRunMain) {
safeUnawaited(_connection.sink.done.then((v) => _doneCompleter.complete()));
}

Expand All @@ -34,6 +35,12 @@ class AppConnection {
if (_startedCompleter.isCompleted) {
throw StateError('Main has already started.');
}

safeUnawaited(_runMain());
}

Future<void> _runMain() async {
await _readyToRunMain;
_connection.sink.add(jsonEncode(serializers.serialize(RunRequest())));
_startedCompleter.complete();
}
Expand Down
2 changes: 1 addition & 1 deletion dwds/lib/src/dwds_vm_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ void _waitForResumeEventToRunMain(
final issuedReadyToRunMainCompleter = Completer<void>();

final resumeEventsSubscription =
chromeProxyService.resumeAfterHotRestartEventsStream.listen((_) async {
chromeProxyService.resumeAfterRestartEventsStream.listen((_) async {
await chromeProxyService.inspector.jsEvaluate('\$dartReadyToRunMain();');
if (!issuedReadyToRunMainCompleter.isCompleted) {
issuedReadyToRunMainCompleter.complete();
Expand Down
50 changes: 46 additions & 4 deletions dwds/lib/src/handlers/dev_handler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,6 @@ class DevHandler {
_servicesByAppId.clear();
}();

bool shouldPauseIsolatesOnStart(String appId) =>
_servicesByAppId[appId]?.chromeProxyService.pauseIsolatesOnStart ?? false;

void _emitBuildResults(BuildResult result) {
if (result.status != BuildStatus.succeeded) return;
for (var injectedConnection in _injectedConnections) {
Expand Down Expand Up @@ -441,7 +438,12 @@ class DevHandler {
// were previously launched and create the new isolate.
final services = _servicesByAppId[message.appId];
final existingConnection = _appConnectionByAppId[message.appId];
final connection = AppConnection(message, sseConnection);
// Completer to indicate when the app's main() method is ready to be run.
// Its future is passed to the AppConnection so that it can be awaited on
// before running the app's main() method:
final readyToRunMainCompleter = Completer<void>();
final connection =
AppConnection(message, sseConnection, readyToRunMainCompleter.future);

// We can take over a connection if there is no connectedInstanceId (this
// means the client completely disconnected), or if the existing
Expand All @@ -460,13 +462,53 @@ class DevHandler {

// Reconnect to existing service.
services.connectedInstanceId = message.instanceId;

if (services.chromeProxyService.pauseIsolatesOnStart) {
// If the pause-isolates-on-start flag is set, we need to wait for
// the resume event to run the app's main() method.
_waitForResumeEventToRunMain(
services.chromeProxyService.resumeAfterRestartEventsStream,
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

readyToRunMainCompleter.complete();
}

await services.chromeProxyService.createIsolate(connection);
} else {
// If this is the initial app connection, we can run the app's main()
// method immediately.
readyToRunMainCompleter.complete();
}
_appConnectionByAppId[message.appId] = connection;
_connectedApps.add(connection);
return connection;
}

/// Waits for a resume event to trigger the app's main() method.
///
/// The [readyToRunMainCompleter]'s future will be passed to the
/// [AppConnection] so that it can be awaited on before running the app's
/// main() method.
void _waitForResumeEventToRunMain(
Stream<String> resumeEventsStream,
Completer<void> readyToRunMainCompleter,
) {
final resumeEventsSubscription = resumeEventsStream.listen((_) {
readyToRunMainCompleter.complete();
if (!readyToRunMainCompleter.isCompleted) {
readyToRunMainCompleter.complete();
}
});

safeUnawaited(
readyToRunMainCompleter.future.then((_) {
resumeEventsSubscription.cancel();
}),
);
}

void _handleIsolateExit(AppConnection appConnection) {
_servicesByAppId[appConnection.request.appId]
?.chromeProxyService
Expand Down
16 changes: 8 additions & 8 deletions dwds/lib/src/services/chrome_proxy_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -99,18 +99,18 @@ class ChromeProxyService implements VmServiceInterface {
/// This value can be updated at runtime via [setFlag].
bool get pauseIsolatesOnStart => _pauseIsolatesOnStart;

final _resumeAfterHotRestartEventsController =
final _resumeAfterRestartEventsController =
StreamController<String>.broadcast();

/// A global stream of resume events.
///
/// The values in the stream are the isolates IDs for the resume event.
///
/// IMPORTANT: This should only be listened to during a hot-restart. The
/// debugger ignores any resume events as long as there is a subscriber to
/// this stream.
Stream<String> get resumeAfterHotRestartEventsStream =>
_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

Stream<String> get resumeAfterRestartEventsStream =>
_resumeAfterRestartEventsController.stream;

final _logger = Logger('ChromeProxyService');

Expand Down Expand Up @@ -1147,8 +1147,8 @@ ${globalToolConfiguration.loadStrategy.loadModuleSnippet}("dart_sdk").developer.
}) async {
// If there is a subscriber listening for a resume event after hot-restart,
// then add the event to the stream and skip processing it.
if (_resumeAfterHotRestartEventsController.hasListener) {
_resumeAfterHotRestartEventsController.add(isolateId);
if (_resumeAfterRestartEventsController.hasListener) {
_resumeAfterRestartEventsController.add(isolateId);
return Success();
}
if (inspector.appConnection.isStarted) {
Expand Down
6 changes: 2 additions & 4 deletions dwds/test/chrome_proxy_service_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2070,9 +2070,8 @@ void main() {
service.setFlag('pause_isolates_on_start', 'true'),
completion(_isSuccess),
);
final appId = context.appConnection.request.appId;
expect(
context.dwds!.shouldPauseIsolatesOnStart(appId),
context.service.pauseIsolatesOnStart,
equals(true),
);
});
Expand All @@ -2083,9 +2082,8 @@ void main() {
service.setFlag('pause_isolates_on_start', 'false'),
completion(_isSuccess),
);
final appId = context.appConnection.request.appId;
expect(
context.dwds!.shouldPauseIsolatesOnStart(appId),
context.service.pauseIsolatesOnStart,
equals(false),
);
});
Expand Down
33 changes: 32 additions & 1 deletion dwds/test/reload_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,8 @@ void main() {
undoEdit();
});

test('does not run app until there is a resume event', () async {
test('after hot-restart, does not run app until there is a resume event',
() async {
await makeEditAndWaitForRebuild();

final eventsDone = expectLater(
Expand Down Expand Up @@ -563,6 +564,36 @@ void main() {
final sourceAfterResume = await context.webDriver.pageSource;
expect(sourceAfterResume.contains(newString), isTrue);
});

test('after page refresh, does not run app until there is a resume event',
() async {
await makeEditAndWaitForRebuild();

await context.webDriver.driver.refresh();

final eventsDone = expectLater(
client.onIsolateEvent,
emitsThrough(
emitsInOrder([
_hasKind(EventKind.kIsolateExit),
_hasKind(EventKind.kIsolateStart),
_hasKind(EventKind.kIsolateRunnable),
]),
),
);

await eventsDone;

final sourceBeforeResume = await context.webDriver.pageSource;
expect(sourceBeforeResume.contains(newString), isFalse);

final vm = await client.getVM();
final isolateId = vm.isolates!.first.id!;
await client.resume(isolateId);

final sourceAfterResume = await context.webDriver.pageSource;
expect(sourceAfterResume.contains(newString), isTrue);
});
});
}

Expand Down
Loading