Skip to content

Commit 84e6ed0

Browse files
DanTupCommit Queue
authored andcommitted
[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 #60128 Change-Id: Ie2679fc806ab3edf007259298da82dbc8b802a6f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/410760 Reviewed-by: Ben Konyi <[email protected]> Commit-Queue: Derek Xu <[email protected]> Commit-Queue: Ben Konyi <[email protected]> Reviewed-by: Derek Xu <[email protected]>
1 parent f4cadd5 commit 84e6ed0

File tree

2 files changed

+29
-13
lines changed

2 files changed

+29
-13
lines changed

pkg/dds/lib/src/dap/adapters/dart.dart

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -832,7 +832,8 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
832832
final pauseEventKind = isolate.runnable ?? false
833833
? vm.EventKind.kIsolateRunnable
834834
: vm.EventKind.kIsolateStart;
835-
await isolateManager.registerIsolate(isolate, pauseEventKind);
835+
final thread =
836+
await isolateManager.registerIsolate(isolate, pauseEventKind);
836837

837838
// If the Isolate already has a Pause event we can give it to the
838839
// IsolateManager to handle (if it's PausePostStart it will re-configure
@@ -844,7 +845,8 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
844845
isolate.pauseEvent!,
845846
);
846847
} else if (isolate.runnable == true) {
847-
await isolateManager.readyToResumeIsolate(isolate);
848+
await isolateManager.handleThreadStartup(thread,
849+
sendStoppedOnEntry: false);
848850
}
849851
}));
850852
}

pkg/dds/lib/src/dap/isolate_manager.dart

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -640,18 +640,9 @@ class IsolateManager {
640640
// after a hot restart.
641641
if (eventKind == vm.EventKind.kPausePostRequest) {
642642
await _configureIsolate(thread);
643-
await readyToResumeThread(thread.threadId);
643+
await handleThreadStartup(thread, sendStoppedOnEntry: false);
644644
} else if (eventKind == vm.EventKind.kPauseStart) {
645-
// Don't resume from a PauseStart if this has already happened (see
646-
// comments on [thread.hasBeenStarted]).
647-
if (!thread.startupHandled) {
648-
thread.startupHandled = true;
649-
// Send a Stopped event to inform the client UI the thread is paused and
650-
// declare that we are ready to resume (which might result in an
651-
// immediate resume).
652-
sendStoppedOnEntryEvent(thread);
653-
await readyToResumeThread(thread.threadId);
654-
}
645+
handleThreadStartup(thread, sendStoppedOnEntry: true);
655646
} else {
656647
// PauseExit, PauseBreakpoint, PauseInterrupted, PauseException
657648
var reason = 'pause';
@@ -719,6 +710,29 @@ class IsolateManager {
719710
}
720711
}
721712

713+
/// Handles thread startup if it has not already been handled.
714+
///
715+
/// This includes sending Stopped-on-Entry and sending a readyToResume.
716+
Future<void> handleThreadStartup(
717+
ThreadInfo thread, {
718+
required bool sendStoppedOnEntry,
719+
}) async {
720+
// Don't resume from a PauseStart if this has already happened (see
721+
// comments on [thread.startupHandled]).
722+
if (thread.startupHandled) {
723+
return;
724+
}
725+
726+
thread.startupHandled = true;
727+
// Send a Stopped event to inform the client UI the thread is paused and
728+
// declare that we are ready to resume (which might result in an
729+
// immediate resume).
730+
if (sendStoppedOnEntry) {
731+
sendStoppedOnEntryEvent(thread);
732+
}
733+
await readyToResumeThread(thread.threadId);
734+
}
735+
722736
/// Handles a resume event from the VM, updating our local state.
723737
void _handleResumed(vm.Event event) {
724738
final isolate = event.isolate!;

0 commit comments

Comments
 (0)