[dds/dap] Fix a race in DAP startup code that caused flaky tests

We start isolates paused so that we can send breakpoints before any code runs. This means we need to resume after initialization is complete.

It's important we don't try to resume multiple times during initialization (regardless of the order of isolate events or whether the isolate was found when we queried for isolates during connection).. This is done with the flag `startupHandled`.

One code path was not taking this flag into account, which meant multiple resumes were still possible. This seemed to occur on Linux during test runs (I've noticed the order of events being different on Linux in the past).

This change extracts the checking of `startupHandled` before sending `readyToResume` and uses it in all places that handle this kind of startup resume.

Fixes https://github.com/dart-lang/sdk/issues/60128

Change-Id: Ie2679fc806ab3edf007259298da82dbc8b802a6f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/410760
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Derek Xu <derekx@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Derek Xu <derekx@google.com>
This commit is contained in:
Danny Tuppeny
2025-02-19 08:23:28 -08:00
committed by Commit Queue
parent f4cadd5a5a
commit 84e6ed0784
2 changed files with 29 additions and 13 deletions
+4 -2
View File
@@ -832,7 +832,8 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
final pauseEventKind = isolate.runnable ?? false
? vm.EventKind.kIsolateRunnable
: vm.EventKind.kIsolateStart;
await isolateManager.registerIsolate(isolate, pauseEventKind);
final thread =
await isolateManager.registerIsolate(isolate, pauseEventKind);
// If the Isolate already has a Pause event we can give it to the
// IsolateManager to handle (if it's PausePostStart it will re-configure
@@ -844,7 +845,8 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
isolate.pauseEvent!,
);
} else if (isolate.runnable == true) {
await isolateManager.readyToResumeIsolate(isolate);
await isolateManager.handleThreadStartup(thread,
sendStoppedOnEntry: false);
}
}));
}
+25 -11
View File
@@ -640,18 +640,9 @@ class IsolateManager {
// after a hot restart.
if (eventKind == vm.EventKind.kPausePostRequest) {
await _configureIsolate(thread);
await readyToResumeThread(thread.threadId);
await handleThreadStartup(thread, sendStoppedOnEntry: false);
} else if (eventKind == vm.EventKind.kPauseStart) {
// Don't resume from a PauseStart if this has already happened (see
// comments on [thread.hasBeenStarted]).
if (!thread.startupHandled) {
thread.startupHandled = true;
// Send a Stopped event to inform the client UI the thread is paused and
// declare that we are ready to resume (which might result in an
// immediate resume).
sendStoppedOnEntryEvent(thread);
await readyToResumeThread(thread.threadId);
}
handleThreadStartup(thread, sendStoppedOnEntry: true);
} else {
// PauseExit, PauseBreakpoint, PauseInterrupted, PauseException
var reason = 'pause';
@@ -719,6 +710,29 @@ class IsolateManager {
}
}
/// Handles thread startup if it has not already been handled.
///
/// This includes sending Stopped-on-Entry and sending a readyToResume.
Future<void> handleThreadStartup(
ThreadInfo thread, {
required bool sendStoppedOnEntry,
}) async {
// Don't resume from a PauseStart if this has already happened (see
// comments on [thread.startupHandled]).
if (thread.startupHandled) {
return;
}
thread.startupHandled = true;
// Send a Stopped event to inform the client UI the thread is paused and
// declare that we are ready to resume (which might result in an
// immediate resume).
if (sendStoppedOnEntry) {
sendStoppedOnEntryEvent(thread);
}
await readyToResumeThread(thread.threadId);
}
/// Handles a resume event from the VM, updating our local state.
void _handleResumed(vm.Event event) {
final isolate = event.isolate!;