[ DDS ] Rework client resume permissions API

This change reworks the client resume permissions API to make it easier
for clients to deal with user provided `--pause-isolates-on-start` and
`--pause-isolates-on-exit` flags.

`requireUserPermissionToResume` should be called by the tool that
launches the Dart process to indicate whether or not the user provided
`--pause-isolates-on-{start,exit}`. The default behavior is to assume
that a tool set these flags for its own use (e.g., resetting breakpoints
after a hot restart in Flutter), where isolates will resume immediately
after each client that has indicated interest in that pause event has
invoked `readyToResume`.

If a user provided one of the previously mentioned flags, isolates will
not immediately resume after each relevant client has invoked
`readyToResume`. Instead, a call to `resume()` must be made to indicate
the user has triggered the resume request instead of tooling. If the
user permissions to resume are changed while the isolate is paused and
all relevant clients have invoked `readyToResume`, the isolate will
automatically resume if the user no longer requires us to wait for a
user resume.

`resume()` now also acts as a "force resume", bypassing any required
permissions set by tooling.

This behavior change is breaking, so the DDS protocol version is being
bumped to 2.0.

`package:dds_service_extensions` has also been updated to include the
following DDS RPCs:

 - `requireUserPermissionToResume`
 - `readyToResume`

Change-Id: Id5f0806b3c56507d39eb00b6305b8896bab13ae7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357420
Reviewed-by: Elliott Brooks <[email protected]>
Commit-Queue: Ben Konyi <[email protected]>

Author: bkonyi

pkg/dds/CHANGELOG.md

@@ -1,3 +1,8 @@
+# 4.0.0
+- Updated DDS protocol to version 2.0.
+- Added `readyToResume` and `requireUserPermissionToResume` RPCs.
+- **Breaking change:** `resume` is now treated as a user-initiated resume request and force resumes paused isolates, regardless of required resume approvals. Tooling relying on resume permissions should use the `readyToResume` RPC to indicate to DDS that they are ready to resume.
+
 # 3.4.0
 - Start the Dart Tooling Daemon from the DevTools server when a connection is not passed to the server on start.
 

pkg/dds/dds_protocol.md

@@ -1,6 +1,6 @@
-# Dart Development Service Protocol 1.5
+# Dart Development Service Protocol 2.0
 
-This document describes _version 1.5_ of the Dart Development Service Protocol.
+This document describes _version 2.0_ of the Dart Development Service Protocol.
 This protocol is an extension of the Dart VM Service Protocol and implements it
 in it's entirety. For details on the VM Service Protocol, see the [Dart VM Service Protocol Specification][service-protocol].
 
@@ -172,6 +172,26 @@ void postEvent(String stream, String eventKind, Map eventData)
 ```
 The _postEvent_ RPC is used to send events to custom Event streams.
 
+### readyToResume
+
+```
+Success readyToResume(string isolateId)
+```
+
+The _readyToResume_ RPC indicates to DDS that the current client is ready
+to resume the isolate.
+
+If the current client requires that approval be given before resuming an
+isolate, this method will:
+
+- Update the approval state for the isolate.
+- Resume the isolate if approval has been given by all clients which
+  require approval.
+
+Returns a collected sentinel if the isolate no longer exists.
+
+See [Success](#success).
+
 ### requirePermissionToResume
 
 ```
@@ -184,6 +204,8 @@ The _requirePermissionToResume_ RPC is used to change the pause/resume behavior
 of isolates by providing a way for the VM service to wait for approval to resume
 from some set of clients. This is useful for clients which want to perform some
 operation on an isolate after a pause without it being resumed by another client.
+These clients should invoke [readyToResume](#readyToResume) instead of [resume](resume)
+to indicate to DDS that they have finished their work and the isolate can be resumed.
 
 If the _onPauseStart_ parameter is `true`, isolates will not resume after pausing
 on start until the client sends a `resume` request and all other clients which
@@ -206,6 +228,30 @@ need to provide resume approval for this pause type have done so.
   already given approval. In the case that no other client requires resume
   approval for the current pause event, the isolate will be resumed if at
   least one other client has attempted to [resume](resume) the isolate.
+- Resume permission behavior can be bypassed using the [VmService.resume]
+  RPC, which is treated as a user-initiated resume that force resumes
+  the isolate. Tooling relying on resume permissions should use
+  [readyToResume](#readyToResume) instead of [resume](resume) to avoid force
+  resuming the isolate.
+
+See [Success](#success).
+
+### requireUserPermissionToResume
+
+```
+Success requireUserPermissionToResume(bool onPauseStart [optional],
+                                      bool onPauseExit [optional])
+```
+
+The _requireUserPermissionToResume_ RPC notifies DDS if it should wait
+for a [resume](resume) request to resume isolates paused on start or
+exit.
+
+This RPC should only be invoked by tooling which launched the target Dart
+process and knows if the user indicated they wanted isolates paused on
+start or exit.
+
+See [Success](#success).
 
 ### setClientName
 

pkg/dds/lib/dds.dart

@@ -166,7 +166,7 @@ abstract class DartDevelopmentService {
 
   /// The version of the DDS protocol supported by this [DartDevelopmentService]
   /// instance.
-  static const String protocolVersion = '1.6';
+  static const String protocolVersion = '2.0';
 }
 
 class DartDevelopmentServiceException implements Exception {

pkg/dds/lib/src/client.dart

@@ -179,6 +179,19 @@ class DartDevelopmentServiceClient {
       (parameters) => dds.isolateManager.resumeIsolate(this, parameters),
     );
 
+    _clientPeer.registerMethod(
+      'readyToResume',
+      (parameters) => dds.isolateManager.readyToResume(this, parameters),
+    );
+
+    _clientPeer.registerMethod(
+      'requireUserPermissionToResume',
+      (parameters) => dds.isolateManager.requireUserPermissionToResume(
+        this,
+        parameters,
+      ),
+    );
+
     _clientPeer.registerMethod('getStreamHistory', (parameters) {
       final stream = parameters['stream'].asString;
       final events = dds.streamManager.getStreamHistory(stream);

pkg/dds/lib/src/client_manager.dart

@@ -81,10 +81,10 @@ class ClientManager {
   }
 
   /// Require permission from this client before resuming an isolate.
-  Map<String, dynamic> requirePermissionToResume(
+  Future<Map<String, dynamic>> requirePermissionToResume(
     DartDevelopmentServiceClient client,
     json_rpc.Parameters parameters,
-  ) {
+  ) async {
     int pauseTypeMask = 0;
     if (parameters['onPauseStart'].asBoolOr(false)) {
       pauseTypeMask |= PauseTypeMasks.pauseOnStartMask;
@@ -95,8 +95,12 @@ class ClientManager {
     if (parameters['onPauseExit'].asBoolOr(false)) {
       pauseTypeMask |= PauseTypeMasks.pauseOnExitMask;
     }
-
     clientResumePermissions[client.name!]!.permissionsMask = pauseTypeMask;
+
+    // Check to see if any isolates should resume as a result of the
+    // resume permissions being updated.
+    await dds.isolateManager.maybeResumeIsolates();
+
     return RPCResponses.success;
   }
 

pkg/dds/lib/src/isolate_manager.dart

@@ -90,6 +90,16 @@ class RunningIsolate {
       // Mark approval by the client.
       _resumeApprovalsByName.add(resumingClient.name);
     }
+
+    // If the user is required to resume the isolate, we won't resume until a
+    // `resume` request is received or `setRequireUserPermissionToResume` is
+    // invoked and removes the requirement for this pause type.
+    final userPermissionMask =
+        isolateManager._requireUserPermissionToResumeMask;
+    if (userPermissionMask & _isolateStateMask != 0) {
+      return false;
+    }
+
     final requiredClientApprovals = <String>{};
     final permissions =
         isolateManager.dds.clientManager.clientResumePermissions;
@@ -293,15 +303,38 @@ class IsolateManager {
     isolates.remove(id);
   }
 
-  /// Handles `resume` RPC requests. If the client requires that approval be
-  /// given before resuming an isolate, this method will:
+  /// Handles `resume` RPC requests.
+  ///
+  /// Invocations of `resume` are treated as user initiated and will bypass any
+  /// resume permissions set by tooling, force resuming the isolate and clear
+  /// any resume approvals.
+  ///
+  /// Returns a collected sentinel if the isolate no longer exists.
+  Future<Map<String, dynamic>> resumeIsolate(
+    DartDevelopmentServiceClient client,
+    json_rpc.Parameters parameters,
+  ) async {
+    return await _mutex.runGuarded(
+      () async {
+        final isolateId = parameters['isolateId'].asString;
+        final isolate = isolates[isolateId];
+        if (isolate == null) {
+          return RPCResponses.collectedSentinel;
+        }
+        return await _resumeCommon(isolate, parameters);
+      },
+    );
+  }
+
+  /// Handles `readyToResume` RPC requests. If the client requires
+  /// that approval be given before resuming an isolate, this method will:
   ///
   ///   - Update the approval state for the isolate.
   ///   - Resume the isolate if approval has been given by all clients which
   ///     require approval.
   ///
   /// Returns a collected sentinel if the isolate no longer exists.
-  Future<Map<String, dynamic>> resumeIsolate(
+  Future<Map<String, dynamic>> readyToResume(
     DartDevelopmentServiceClient client,
     json_rpc.Parameters parameters,
   ) async {
@@ -313,14 +346,75 @@ class IsolateManager {
           return RPCResponses.collectedSentinel;
         }
         if (isolate.shouldResume(resumingClient: client)) {
-          isolate.clearResumeApprovals();
-          return await _sendResumeRequest(isolateId, parameters);
+          return await _resumeCommon(isolate, parameters);
         }
         return RPCResponses.success;
       },
     );
   }
 
+  Future<void> maybeResumeIsolates() async {
+    await _mutex.runGuarded(() async {
+      for (final isolate in isolates.values) {
+        if (isolate.shouldResume()) {
+          await _resumeCommon(isolate, json_rpc.Parameters('', {}));
+        }
+      }
+    });
+  }
+
+  Future<Map<String, dynamic>> _resumeCommon(
+    RunningIsolate isolate,
+    json_rpc.Parameters parameters,
+  ) async {
+    isolate.clearResumeApprovals();
+    return await _sendResumeRequest(isolate.id, parameters);
+  }
+
+  /// Handles `requireUserPermissionToResume` requests.
+  ///
+  /// Notifies DDS if it should wait for a `resume` request to resume isolates
+  /// paused on start or exit.
+  ///
+  /// This RPC should only be invoked by tooling which launched the target Dart
+  /// process and knows if the user indicated they wanted isolates paused on
+  /// start or exit.
+  Future<Map<String, dynamic>> requireUserPermissionToResume(
+    DartDevelopmentServiceClient client,
+    json_rpc.Parameters parameters,
+  ) async {
+    int pauseTypeMask = 0;
+    if (parameters['onPauseStart'].asBoolOr(false)) {
+      pauseTypeMask |= PauseTypeMasks.pauseOnStartMask;
+    }
+    if (parameters['onPauseExit'].asBoolOr(false)) {
+      pauseTypeMask |= PauseTypeMasks.pauseOnExitMask;
+    }
+    _requireUserPermissionToResumeMask = pauseTypeMask;
+
+    // Check if isolates have been waiting for a user resume and resume any
+    // isolates that no longer need to wait for a user resume.
+    await maybeResumeIsolates();
+
+    return RPCResponses.success;
+  }
+
+  /// Handles `getRequireUserPermissionToResume` requests.
+  ///
+  /// Returns an object indicating whether or not a `resume` request is
+  /// required for DDS to resume an isolate paused on start or exit.
+  Future<Map<String, dynamic>> getRequireUserPermissionToResume(
+      DartDevelopmentServiceClient client,
+      json_rpc.Parameters parameters) async {
+    bool flagFromMask(int mask) =>
+        _requireUserPermissionToResumeMask & mask != 0;
+    return <String, dynamic>{
+      'type': 'ResumePermissionsRequired',
+      'onPauseStart': flagFromMask(PauseTypeMasks.pauseOnStartMask),
+      'onPauseExit': flagFromMask(PauseTypeMasks.pauseOnExitMask),
+    };
+  }
+
   Future<Map<String, dynamic>> getCachedCpuSamples(
       json_rpc.Parameters parameters) async {
     final isolateId = parameters['isolateId'].asString;
@@ -381,5 +475,6 @@ class IsolateManager {
   bool _initialized = false;
   final DartDevelopmentServiceImpl dds;
   final _mutex = Mutex();
-  final Map<String, RunningIsolate> isolates = {};
+  int _requireUserPermissionToResumeMask = 0;
+  final isolates = <String, RunningIsolate>{};
 }

pkg/dds/pubspec.yaml

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

pkg/dds/test/client_resume_approvals_approve_then_disconnect_test.dart

@@ -2,6 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:dds_service_extensions/dds_service_extensions.dart';
 import 'package:vm_service/vm_service.dart';
 
 import 'client_resume_approvals_common.dart';
@@ -34,7 +35,7 @@ final test = <IsolateTest>[
 
     // Give resume approval for client1 to ensure approval state is cleaned up
     // properly when both client1 and client2 have disconnected.
-    await client1.resume(isolateId);
+    await client1.readyToResume(isolateId);
     await hasPausedAtStart(service, isolate);
 
     // Once client1 is disconnected, we should still be paused.
@@ -47,7 +48,7 @@ final test = <IsolateTest>[
     client2.dispose();
     await hasPausedAtStart(service, isolate);
 
-    await service.resume(isolateId);
+    await service.readyToResume(isolateId);
   },
   hasStoppedAtExit,
 ];

pkg/dds/test/client_resume_approvals_disconnect_test.dart

@@ -2,6 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:dds_service_extensions/dds_service_extensions.dart';
 import 'package:vm_service/vm_service.dart';
 
 import 'client_resume_approvals_common.dart';
@@ -34,7 +35,7 @@ final test = <IsolateTest>[
 
     // Send a resume request on the test client so we'll resume once the other
     // clients which require approval disconnect.
-    await service.resume(isolateId);
+    await service.readyToResume(isolateId);
     await hasPausedAtStart(service, isolate);
 
     // Once client1 is disconnected, we should still be paused.

pkg/dds/test/client_resume_approvals_force_resume.dart

@@ -0,0 +1,56 @@
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:dds_service_extensions/dds_service_extensions.dart';
+import 'package:vm_service/vm_service.dart';
+
+import 'client_resume_approvals_common.dart';
+import 'common/service_test_common.dart';
+import 'common/test_helper.dart';
+
+const String clientName = 'TestClient';
+const String otherClientName = 'OtherTestClient';
+
+void fooBar() {
+  int i = 0;
+  print(i);
+}
+
+final test = <IsolateTest>[
+  // Multiple clients, different client names.
+  (VmService service, IsolateRef isolateRef) async {
+    final isolateId = isolateRef.id!;
+    final client1 = await createClient(
+      service: service,
+      clientName: clientName,
+      onPauseStart: true,
+    );
+    // ignore: unused_local_variable
+    final client2 = await createClient(
+      service: service,
+      clientName: otherClientName,
+      onPauseStart: true,
+    );
+
+    await hasPausedAtStart(service, isolateRef);
+    await client1.requireUserPermissionToResume(
+      onPauseStart: true,
+    );
+
+    // Invocations of `resume` are considered to be resume requests made by the
+    // user and is treated as a force resume, ignoring any resume permissions
+    // set by clients.
+    await client1.resume(isolateId);
+    await hasStoppedAtExit(service, isolateRef);
+  },
+];
+
+void main([args = const <String>[]]) => runIsolateTests(
+      args,
+      test,
+      'client_resume_approvals_force_resume.dart',
+      testeeConcurrent: fooBar,
+      pauseOnStart: true,
+      pauseOnExit: true,
+    );