Adding Tests, DartDoc, and Lints to Service Extension Router

This CL is a polish step in preparation for publishing SER.



Change-Id: If1222e3b63270362c25fa076e6adc01fe4938ed5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/335244
Reviewed-by: Ben Konyi <[email protected]>
Commit-Queue: Dan Chevalier <[email protected]>

pkg/service_extension_router/analysis_options.yaml

@@ -1,3 +1,6 @@
-
 include: package:lints/recommended.yaml
 
+linter:
+  rules:
+    - avoid_void_async
+    - unawaited_futures

pkg/service_extension_router/lib/service_extension_router.dart

@@ -6,4 +6,3 @@ library;
 
 export 'src/stream_manager.dart';
 export 'src/client.dart';
-export 'src/client_manager.dart';

pkg/service_extension_router/lib/src/client.dart

@@ -2,13 +2,29 @@
 // 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.
 
-// TODO(danchevalier): Add documentation before major release
+import 'package:service_extension_router/src/named_lookup.dart';
+import 'package:meta/meta.dart';
+
+/// Represents a client that connects to a service.
 abstract class Client {
+  /// Called when this client is receiving [data] from [stream].
+  ///
+  /// This method should do any formatting needed on [data], then send it to
+  /// the [Client].
   void streamNotify(String stream, Map<String, Object?> data);
 
+  /// Called if the connection to the client should be closed.
   Future<void> close();
+
+  /// Called when a request should be sent to this client.
+  ///
+  /// This method should forward [method] with [parameters] to the client.
   Future<dynamic> sendRequest({required String method, dynamic parameters});
 
+  /// A list of services that are handled by this client.
+  ///
+  /// The key is intended to be a service and the value is intended to be an
+  /// alias.
   final Map<String, String> services = {};
 
   static int _idCounter = 0;
@@ -23,6 +39,94 @@ abstract class Client {
   // NOTE: this should not be called directly except from:
   //   - `ClientManager._clearClientName`
   //   - `ClientManager._setClientNameHelper`
-  set name(String? n) => _name = n ?? defaultClientName;
+  void _setName(String? n) => _name = n ?? defaultClientName;
   String? _name;
 }
+
+/// Used for keeping track and managing clients that are connected to a given
+/// service.
+///
+/// Call [addClient] when a client connects to your service, then call
+/// [removeClient] when it stops listening.
+abstract class ClientManager {
+  /// Adds [client] to the list of connected clients.
+  ///
+  /// This should be called when a client connects to the service.
+  @mustCallSuper
+  void addClient(Client client) {
+    setClientName(
+      client,
+      client.defaultClientName,
+    );
+    clients.add(client);
+  }
+
+  /// Removes [client] from the list of connected clients.
+  ///
+  /// This should be called when the client disconnects from the service.
+  @mustCallSuper
+  void removeClient(Client client) {
+    clients.remove(client);
+  }
+
+  /// Returns true if the client manager has and clients still connected.
+  bool hasClients() => clients.isNotEmpty;
+
+  /// Cleans up clients that are still connected by calling [Client.close] on
+  /// all of them.
+  Future<void> shutdown() async {
+    // Close all incoming websocket connections.
+    final futures = <Future>[];
+    // Copy `clients` to guard against modification while iterating.
+    for (final client in clients.toList()) {
+      futures.add(
+        Future.sync(() => removeClient(client))
+            .whenComplete(() => client.close()),
+      );
+    }
+    await Future.wait(futures);
+  }
+
+  /// Returns the first client that contains a service, from the list of
+  /// connected clients.
+  @mustCallSuper
+  Client? findFirstClientThatHandlesService(String service) {
+    for (final client in clients) {
+      if (client.services.containsKey(service)) {
+        return client;
+      }
+    }
+    return null;
+  }
+
+  /// Associates a name with a given client.
+  @mustCallSuper
+  void setClientName(
+    Client client,
+    String name,
+  ) {
+    _setClientNameHelper(client, name);
+  }
+
+  /// Changes [client]'s name to [name]
+  void _setClientNameHelper(
+    Client client,
+    String name,
+  ) {
+    clearClientName(client);
+    client._setName(name.isEmpty ? client.defaultClientName : name);
+  }
+
+  static const _kServicePrologue = 's';
+  final NamedLookup<Client> clients = NamedLookup(
+    prologue: _kServicePrologue,
+  );
+
+  /// Unsets a client's name by setting it to null.
+  @mustCallSuper
+  String? clearClientName(Client client) {
+    String? name = client.name;
+    client._setName(null);
+    return name;
+  }
+}

pkg/service_extension_router/lib/src/stream_manager.dart

@@ -6,7 +6,7 @@ import 'package:meta/meta.dart';
 
 import 'client.dart';
 
-// TODO(danchevalier): Add documentation before major release
+/// Facilitates Stream behaviour between [Client]s.
 abstract class StreamManager {
   final _streamListeners =
       <String, List<Client>>{}; // TODO should this be set of StreamClient?
@@ -38,12 +38,9 @@ abstract class StreamManager {
     }
   }
 
-  /// Subscribes `client` to a stream.
-  ///
-  /// If `client` is the first client to listen to `stream`, DDS will send a
-  /// `streamListen` request for `stream` to the VM service.
+  /// Called when a `client`  subscribes to a stream.
   @mustCallSuper
-  void streamListen(
+  Future<void> streamListen(
     Client client,
     String stream,
   ) async {

pkg/service_extension_router/test/client_test.dart

@@ -0,0 +1,121 @@
+// Copyright (c) 2023, 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:service_extension_router/src/client.dart';
+import 'package:test/test.dart';
+
+class TestClientManager extends ClientManager {}
+
+class TestClient extends Client {
+  int closeCount = 0;
+  int sendRequestCount = 0;
+  int streamNotifyCount = 0;
+
+  @override
+  Future<void> close() {
+    closeCount++;
+    return Future.value();
+  }
+
+  @override
+  Future sendRequest({required String method, parameters}) {
+    sendRequestCount++;
+    return Future.value();
+  }
+
+  @override
+  void streamNotify(String stream, Map<String, Object?> data) {
+    streamNotifyCount++;
+  }
+}
+
+void main() {
+  group('ClientManager', () {
+    late ClientManager clientManager;
+    setUp(() {
+      clientManager = TestClientManager();
+    });
+    test('add and remove client', () {
+      final client = TestClient();
+
+      expect(clientManager.clients.length, 0);
+
+      clientManager.addClient(client);
+
+      expect(clientManager.clients.length, 1);
+      expect(clientManager.clients.first, client);
+
+      clientManager.removeClient(client);
+
+      expect(clientManager.clients.length, 0);
+    });
+
+    test('client name', () {
+      final client = TestClient();
+
+      // Name unset until client is added to a manager
+      expect(client.name, isNull);
+
+      // Sets the client name to a default
+      clientManager.addClient(client);
+      final defaultClientName = client.name;
+      expect(client.name?.startsWith('client'), isTrue);
+
+      clientManager.setClientName(client, 'test');
+      expect(client.name, 'test');
+
+      clientManager.clearClientName(client);
+      expect(client.name, defaultClientName);
+    });
+
+    test('findFirstClientThatHandlesService', () {
+      final client1 = TestClient();
+      final client2 = TestClient();
+      client1.services['service1'] = 'alias1';
+      client2.services['service2'] = 'alias2';
+      clientManager.addClient(client1);
+      clientManager.addClient(client2);
+
+      expect(
+        clientManager.findFirstClientThatHandlesService('service1'),
+        client1,
+      );
+      expect(
+        clientManager.findFirstClientThatHandlesService('service2'),
+        client2,
+      );
+    });
+
+    test('shutdown', () async {
+      final client1 = TestClient();
+      final client2 = TestClient();
+      final client3 = TestClient();
+      clientManager.addClient(client1);
+      clientManager.addClient(client2);
+      clientManager.addClient(client3);
+
+      expect(client1.closeCount, 0);
+      expect(client2.closeCount, 0);
+      expect(client3.closeCount, 0);
+      expect(
+        clientManager.clients,
+        unorderedEquals([
+          client1,
+          client2,
+          client3,
+        ]),
+      );
+
+      await clientManager.shutdown();
+
+      expect(client1.closeCount, 1);
+      expect(client2.closeCount, 1);
+      expect(client3.closeCount, 1);
+      expect(
+        clientManager.clients,
+        [],
+      );
+    });
+  });
+}