[dds/dap] Use Isolate numbers as DAP thread IDs

We used to generate our own thread ID starting at 1 and counting up. There was always a 1:1 mapping from a DAP thread to a VM Isolate. With this change, we always use the Isolate number as the thread ID which makes it easier for tools using both VM Service and DAP at the same time.

Change-Id: Id8d82f6fd5134987f2ecfeaa761765d55999405d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/312906
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>

pkg/dds/CHANGELOG.md

@@ -1,3 +1,6 @@
+# 2.9.3
+- [DAP] `threadId`s generated by the debug adapter now match the Isolate numbers of the underlying isolates.
+
 # 2.9.2
 - [DAP] Fixed an issue that could cause breakpoints to become unresolved when there are multiple isolates (such as during a test run).
 - [DAP] Fixed an issue where stack frames parsed in test failures could produce incorrect absolute paths in the `source.path` field if the working directory of the debug adapter did not match that of the launch/attach request.

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

@@ -794,7 +794,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
         if (isolateManager.autoResumeStartingIsolates) {
           await isolateManager.resumeIsolate(isolate);
         } else {
-          isolateManager.sendStoppedOnEntryEvent(thread.threadId);
+          isolateManager.sendStoppedOnEntryEvent(thread.isolateNumber);
         }
       }
     }));
@@ -1599,19 +1599,19 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     //  requests can be used to enforce paging in the client."
     const stackFrameBatchSize = 20;
 
-    final threadId = args.threadId;
-    final thread = isolateManager.getThread(threadId);
+    final isolateNumber = args.threadId;
+    final thread = isolateManager.getThread(isolateNumber);
     final topFrame = thread?.pauseEvent?.topFrame;
     final startFrame = args.startFrame ?? 0;
     final numFrames = args.levels ?? 0;
     var totalFrames = 1;
 
     if (thread == null) {
-      throw DebugAdapterException('No thread with threadId $threadId');
+      throw DebugAdapterException('No thread with threadId $isolateNumber');
     }
 
     if (!thread.paused) {
-      throw DebugAdapterException('Thread $threadId is not paused');
+      throw DebugAdapterException('Thread $isolateNumber is not paused');
     }
 
     final stackFrames = <StackFrame>[];
@@ -1761,7 +1761,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     final threads = [
       for (final thread in isolateManager.threads)
         Thread(
-          id: thread.threadId,
+          id: thread.isolateNumber,
           name: thread.isolate.name ?? '<unnamed isolate>',
         )
     ];

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

@@ -24,8 +24,16 @@ class IsolateManager {
   final DartDebugAdapter _adapter;
   final Map<String, Completer<void>> _isolateRegistrations = {};
   final Map<String, ThreadInfo> _threadsByIsolateId = {};
-  final Map<int, ThreadInfo> _threadsByThreadId = {};
-  int _nextThreadNumber = 1;
+  final Map<int, ThreadInfo> _threadsByIsolateNumber = {};
+
+  /// Tracks isolate numbers that have been seen (even if the isolate has since
+  /// exited).
+  ///
+  /// This is used to know whether a request from a DAP client references a
+  /// thread that has exited rather than an invalid number.
+  ///
+  /// DAP's "thread ID"s map directly onto VM Isolate Numbers.
+  final Set<int> _knownIsolateNumbers = {};
 
   /// Whether debugging is enabled for this session.
   ///
@@ -145,7 +153,7 @@ class IsolateManager {
   /// This is required if options like debugSdkLibraries are modified, but is a
   /// separate step to batch together changes to multiple options.
   Future<void> applyDebugOptions() async {
-    await Future.wait(_threadsByThreadId.values.map(
+    await Future.wait(_threadsByIsolateId.values.map(
       // debuggable libraries is the only thing currently affected by these
       // changable options.
       (thread) => _sendLibraryDebuggables(thread),
@@ -173,7 +181,8 @@ class IsolateManager {
     return _storedData[id];
   }
 
-  ThreadInfo? getThread(int threadId) => _threadsByThreadId[threadId];
+  ThreadInfo? getThread(int isolateNumber) =>
+      _threadsByIsolateNumber[isolateNumber];
 
   /// Handles Isolate and Debug events.
   Future<void> handleEvent(vm.Event event) async {
@@ -225,11 +234,13 @@ class IsolateManager {
       isolate.id!,
       () {
         // The first time we see an isolate, start tracking it.
-        final info = ThreadInfo(this, _nextThreadNumber++, isolate);
-        _threadsByThreadId[info.threadId] = info;
+        final isolateNumber = int.parse(isolate.number!);
+        _knownIsolateNumbers.add(isolateNumber);
+        final info = ThreadInfo(this, isolateNumber, isolate);
+        _threadsByIsolateNumber[isolateNumber] = info;
         // And notify the client about it.
         _adapter.sendEvent(
-          ThreadEventBody(reason: 'started', threadId: info.threadId),
+          ThreadEventBody(reason: 'started', threadId: isolateNumber),
         );
         return info;
       },
@@ -248,7 +259,7 @@ class IsolateManager {
 
   /// Calls reloadSources for all isolates.
   Future<void> reloadSources() async {
-    await Future.wait(_threadsByThreadId.values.map(
+    await Future.wait(_threadsByIsolateId.values.map(
       (isolate) => _reloadSources(isolate.isolate),
     ));
   }
@@ -261,10 +272,10 @@ class IsolateManager {
       return;
     }
 
-    await resumeThread(thread.threadId);
+    await resumeThread(thread.isolateNumber);
   }
 
-  /// Resumes (or steps) an isolate using its client [threadId].
+  /// Resumes (or steps) an isolate using its client [isolateNumber].
   ///
   /// If the isolate is not paused, or already has a pending resume request
   /// in-flight, a request will not be sent.
@@ -272,16 +283,16 @@ class IsolateManager {
   /// If the isolate is paused at an async suspension and the [resumeType] is
   /// [vm.StepOption.kOver], a [StepOption.kOverAsyncSuspension] step will be
   /// sent instead.
-  Future<void> resumeThread(int threadId, [String? resumeType]) async {
+  Future<void> resumeThread(int isolateNumber, [String? resumeType]) async {
     // The first time a user resumes a thread is our signal that the app is now
     // "running" and future isolates can be auto-resumed. This only affects
     // attach, as it's already `true` for launch requests.
     autoResumeStartingIsolates = true;
 
-    final thread = _threadsByThreadId[threadId];
+    final thread = _threadsByIsolateNumber[isolateNumber];
     if (thread == null) {
-      if (isInvalidThreadId(threadId)) {
-        throw DebugAdapterException('Thread $threadId was not found');
+      if (isInvalidThreadId(isolateNumber)) {
+        throw DebugAdapterException('Thread $isolateNumber was not found');
       } else {
         // Otherwise, this thread has exited and we don't need to do anything.
         // It's possible another debugger unpaused or we're shutting down and
@@ -326,16 +337,16 @@ class IsolateManager {
     }
   }
 
-  /// Pauses an isolate using its client [threadId].
+  /// Pauses an isolate using its client [isolateNumber].
   ///
   /// This is simply a _request_ to pause. It does not change any state by
   /// itself - we will handle the pause via an event if the pause request
   /// succeeds.
-  Future<void> pauseThread(int threadId) async {
-    final thread = _threadsByThreadId[threadId];
+  Future<void> pauseThread(int isolateNumber) async {
+    final thread = _threadsByIsolateNumber[isolateNumber];
     if (thread == null) {
-      if (isInvalidThreadId(threadId)) {
-        throw DebugAdapterException('Thread $threadId was not found');
+      if (isInvalidThreadId(isolateNumber)) {
+        throw DebugAdapterException('Thread $isolateNumber was not found');
       } else {
         // Otherwise, this thread has recently exited so we cannot attempt
         // to pause it.
@@ -352,15 +363,18 @@ class IsolateManager {
     }
   }
 
-  /// Checks whether [threadId] is invalid and has never been used.
+  /// Checks whether [isolateNumber] is invalid and has never been used.
   ///
-  /// Returns `false` is [threadId] corresponds to either a live, or previously
+  /// Returns `false` is [isolateNumber] corresponds to either a live, or previously
   /// exited thread.
-  bool isInvalidThreadId(int threadId) => threadId >= _nextThreadNumber;
+  bool isInvalidThreadId(int isolateNumber) =>
+      !_knownIsolateNumbers.contains(isolateNumber);
 
   /// Sends an event informing the client that a thread is stopped at entry.
-  void sendStoppedOnEntryEvent(int threadId) {
-    _adapter.sendEvent(StoppedEventBody(reason: 'entry', threadId: threadId));
+  void sendStoppedOnEntryEvent(int isolateNumber) {
+    _adapter.sendEvent(
+      StoppedEventBody(reason: 'entry', threadId: isolateNumber),
+    );
   }
 
   /// Records breakpoints for [uri].
@@ -375,7 +389,7 @@ class IsolateManager {
     _clientBreakpointsByUri[uri] = breakpoints;
 
     // Send the breakpoints to all existing threads.
-    await Future.wait(_threadsByThreadId.values
+    await Future.wait(_threadsByIsolateNumber.values
         .map((thread) => _sendBreakpoints(thread, uri: uri)));
   }
 
@@ -387,7 +401,7 @@ class IsolateManager {
 
     // Send the breakpoints to all existing threads.
     await Future.wait(
-      _threadsByThreadId.values.map((thread) => _sendBreakpoints(thread)),
+      _threadsByIsolateNumber.values.map((thread) => _sendBreakpoints(thread)),
     );
   }
 
@@ -397,7 +411,7 @@ class IsolateManager {
     _exceptionPauseMode = mode;
 
     // Send to all existing threads.
-    await Future.wait(_threadsByThreadId.values.map(
+    await Future.wait(_threadsByIsolateNumber.values.map(
       (thread) => _sendExceptionPauseMode(thread),
     ));
   }
@@ -496,10 +510,10 @@ class IsolateManager {
     if (thread != null) {
       // Notify the client.
       _adapter.sendEvent(
-        ThreadEventBody(reason: 'exited', threadId: thread.threadId),
+        ThreadEventBody(reason: 'exited', threadId: thread.isolateNumber),
       );
       _threadsByIsolateId.remove(isolateId);
-      _threadsByThreadId.remove(thread.threadId);
+      _threadsByIsolateNumber.remove(thread.isolateNumber);
     }
   }
 
@@ -536,7 +550,7 @@ class IsolateManager {
     if (eventKind == vm.EventKind.kPausePostRequest) {
       await _configureIsolate(thread);
       if (autoResumeStartingIsolates) {
-        await resumeThread(thread.threadId);
+        await resumeThread(thread.isolateNumber);
       }
     } else if (eventKind == vm.EventKind.kPauseStart) {
       // Don't resume from a PauseStart if this has already happened (see
@@ -546,9 +560,9 @@ class IsolateManager {
         // If requested, automatically resume. Otherwise send a Stopped event to
         // inform the client UI the thread is paused.
         if (autoResumeStartingIsolates) {
-          await resumeThread(thread.threadId);
+          await resumeThread(thread.isolateNumber);
         } else {
-          sendStoppedOnEntryEvent(thread.threadId);
+          sendStoppedOnEntryEvent(thread.isolateNumber);
         }
       }
     } else {
@@ -583,7 +597,7 @@ class IsolateManager {
         // breakpoints don't have false conditions.
         if (breakpoints.isEmpty ||
             !await _shouldHitBreakpoint(thread, breakpoints)) {
-          await resumeThread(thread.threadId);
+          await resumeThread(thread.isolateNumber);
           return;
         }
       } else if (eventKind == vm.EventKind.kPauseBreakpoint) {
@@ -606,7 +620,7 @@ class IsolateManager {
       _adapter.sendEvent(
         StoppedEventBody(
           reason: reason,
-          threadId: thread.threadId,
+          threadId: thread.isolateNumber,
           text: text,
         ),
       );
@@ -765,7 +779,7 @@ class IsolateManager {
   Future<void> resumeAll() async {
     final pausedThreads = threads.where((thread) => thread.paused).toList();
     await Future.wait(
-      pausedThreads.map((thread) => resumeThread(thread.threadId)),
+      pausedThreads.map((thread) => resumeThread(thread.isolateNumber)),
     );
   }
 
@@ -973,7 +987,7 @@ class IsolateManager {
 class ThreadInfo {
   final IsolateManager _manager;
   final vm.IsolateRef isolate;
-  final int threadId;
+  final int isolateNumber;
   var runnable = false;
   var atAsyncSuspension = false;
   int? exceptionReference;
@@ -1020,7 +1034,7 @@ class ThreadInfo {
   /// been responded to.
   var hasPendingResume = false;
 
-  ThreadInfo(this._manager, this.threadId, this.isolate);
+  ThreadInfo(this._manager, this.isolateNumber, this.isolate);
 
   Future<T> getObject<T extends vm.Response>(vm.ObjRef ref) =>
       _manager.getObject<T>(isolate, ref);

pkg/dds/pubspec.yaml

@@ -1,5 +1,5 @@
 name: dds
-version: 2.9.2
+version: 2.9.3
 description: >-
   A library used to spawn the Dart Developer Service, used to communicate with
   a Dart VM Service instance.

pkg/dds/test/dap/integration/debug_variables_test.dart

@@ -795,7 +795,7 @@ void main() {
       final stop = await client.hitBreakpoint(testFile, breakpointLine);
 
       await client.expectScopeVariables(
-        stop.threadId!,
+        await client.getTopFrameId(stop.threadId!),
         'Locals',
         'foo: <not initialized>',
       );

pkg/dds/test/dap/isolate_manager_test.dart

@@ -114,7 +114,7 @@ main() {
 
       // Resume thread1
       thread1.paused = true; // Fake pause to allow resume.
-      await isolateManager.resumeThread(thread1.threadId);
+      await isolateManager.resumeThread(thread1.isolateNumber);
 
       // Ensure thread1 had data cleared, but thread2 did not.
       expect(isolateManager.getStoredData(ref1), isNull);

pkg/dds/test/dap/mocks.dart

@@ -133,8 +133,8 @@ class MockVmService implements VmServiceInterface {
   @override
   dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
 
-  final isolate1 = IsolateRef(id: 'isolate1');
-  final isolate2 = IsolateRef(id: 'isolate2');
+  final isolate1 = IsolateRef(id: 'isolate1', number: '1');
+  final isolate2 = IsolateRef(id: 'isolate2', number: '2');
   final sdkLibrary = LibraryRef(id: 'libSdk', uri: 'dart:core');
   final externalPackageLibrary =
       LibraryRef(id: 'libPkgExternal', uri: 'package:external/foo.dart');