[ Service / DDS ] Remove VM service polling logic, add --enable-service-fallback-port support to DDS

Polling logic was introduced due to a bug in Fuchsia's network stack
which could result in the VM service attempting to start the server
before the network stack was initialized. This issue should be resolved
now, so this logic is no longer necessary.

TEST=pkg/dartdev/test/commands/run_test.dart

Change-Id: I10f185dfb1be1b0363983f3e0564d65c38c99ea8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/235763
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>

pkg/dartdev/lib/src/commands/run.dart

@@ -222,6 +222,8 @@ class RunCommand extends DartdevCommand {
       final bool debugDds = args['debug-dds'];
 
       bool disableServiceAuthCodes = args['disable-service-auth-codes'];
+      final bool enableServicePortFallback =
+          args['enable-service-port-fallback'];
 
       // If the user wants to start a debugging session we need to do some extra
       // work and spawn a Dart Development Service (DDS) instance. DDS is a VM
@@ -237,6 +239,7 @@ class RunCommand extends DartdevCommand {
           disableServiceAuthCodes,
           launchDevTools,
           debugDds,
+          enableServicePortFallback,
         )) {
           return errorExitCode;
         }
@@ -274,6 +277,7 @@ class _DebuggingSession {
     bool disableServiceAuthCodes,
     bool enableDevTools,
     bool debugDds,
+    bool enableServicePortFallback,
   ) async {
     final sdkDir = dirname(sdk.dart);
     final fullSdk = sdkDir.endsWith('bin');
@@ -303,6 +307,7 @@ class _DebuggingSession {
         enableDevTools.toString(),
         devToolsBinaries,
         debugDds.toString(),
+        enableServicePortFallback.toString(),
       ],
       mode: ProcessStartMode.detachedWithStdio,
     );

pkg/dartdev/test/commands/run_test.dart

@@ -333,6 +333,25 @@ void main(List<String> args) => print("$b $args");
     expect(result.exitCode, 0);
   });
 
+  test('--enable-service-port-fallback', () async {
+    final p = project(mainSrc: '''void main() {}''');
+    final server = await HttpServer.bind(InternetAddress.loopbackIPv4, 0);
+    final result = await p.run(
+      [
+        'run',
+        '--enable-vm-service=${server.port}',
+        '--enable-service-port-fallback',
+        p.relativeFilePath,
+      ],
+    );
+    final regexp = RegExp(
+        r'Observatory listening on http:\/\/127.0.0.1:(\d*)\/[a-zA-Z0-9_-]+=\/\n.*');
+    final vmServicePort =
+        int.parse(regexp.firstMatch(result.stdout)!.group(1)!);
+    expect(server.port != vmServicePort, isTrue);
+    await server.close();
+  });
+
   test('without verbose CFE info', () async {
     final p = project(mainSrc: '''void main() {}''');
 

pkg/dds/bin/dds.dart

@@ -18,6 +18,7 @@ import 'package:dds/dds.dart';
 ///   - Start DevTools
 ///   - DevTools build directory
 ///   - Enable logging
+///   - Enable service port fallback
 Future<void> main(List<String> args) async {
   if (args.isEmpty) return;
 
@@ -46,6 +47,7 @@ Future<void> main(List<String> args) async {
     devToolsBuildDirectory = Uri.file(args[5]);
   }
   final logRequests = args[6] == 'true';
+  final enableServicePortFallback = args[7] == 'true';
   try {
     // TODO(bkonyi): add retry logic similar to that in vmservice_server.dart
     // See https://github.com/dart-lang/sdk/issues/43192.
@@ -61,6 +63,7 @@ Future<void> main(List<String> args) async {
             )
           : null,
       logRequests: logRequests,
+      enableServicePortFallback: enableServicePortFallback,
     );
     stderr.write(json.encode({
       'state': 'started',

pkg/dds/lib/dds.dart

@@ -37,11 +37,15 @@ abstract class DartDevelopmentService {
   ///
   /// [ipv6] controls whether or not DDS is served via IPv6. IPv4 is enabled by
   /// default.
+  ///
+  /// If [enablesServicePortFallback] is enabled, DDS will attempt to bind to any
+  /// available port if the specified port is unavailable.
   static Future<DartDevelopmentService> startDartDevelopmentService(
     Uri remoteVmServiceUri, {
     Uri? serviceUri,
     bool enableAuthCodes = true,
     bool ipv6 = false,
+    bool enableServicePortFallback = false,
     List<String> cachedUserTags = const [],
     DevToolsConfiguration? devToolsConfiguration,
     bool logRequests = false,
@@ -84,6 +88,7 @@ abstract class DartDevelopmentService {
       ipv6,
       devToolsConfiguration,
       logRequests,
+      enableServicePortFallback,
     );
     await service.startService();
     return service;

pkg/dds/lib/src/dds_impl.dart

@@ -59,6 +59,7 @@ class DartDevelopmentServiceImpl implements DartDevelopmentService {
     this._ipv6,
     this._devToolsConfiguration,
     this.shouldLogRequests,
+    this._enableServicePortFallback,
   ) {
     _clientManager = ClientManager(this);
     _expressionEvaluator = ExpressionEvaluator(this);
@@ -136,7 +137,7 @@ class DartDevelopmentServiceImpl implements DartDevelopmentService {
     final host = uri?.host ??
         (_ipv6 ? InternetAddress.loopbackIPv6 : InternetAddress.loopbackIPv4)
             .host;
-    final port = uri?.port ?? 0;
+    var port = uri?.port ?? 0;
     var pipeline = const Pipeline();
     if (shouldLogRequests) {
       pipeline = pipeline.addMiddleware(
@@ -155,16 +156,25 @@ class DartDevelopmentServiceImpl implements DartDevelopmentService {
     late String errorMessage;
     final tmpServer = await runZonedGuarded(
       () async {
-        try {
-          return await io.serve(handler, host, port);
-        } on SocketException catch (e) {
-          errorMessage = e.message;
-          if (e.osError != null) {
-            errorMessage += ' (${e.osError!.message})';
+        Future<HttpServer?> startServer() async {
+          try {
+            return await io.serve(handler, host, port);
+          } on SocketException catch (e) {
+            if (_enableServicePortFallback && port != 0) {
+              // Try again, this time with a random port.
+              port = 0;
+              return await startServer();
+            }
+            errorMessage = e.message;
+            if (e.osError != null) {
+              errorMessage += ' (${e.osError!.message})';
+            }
+            errorMessage += ': ${e.address?.host}:${e.port}';
+            return null;
           }
-          errorMessage += ': ${e.address?.host}:${e.port}';
-          return null;
         }
+
+        return await startServer();
       },
       (error, stack) {
         if (shouldLogRequests) {
@@ -387,6 +397,7 @@ class DartDevelopmentServiceImpl implements DartDevelopmentService {
   String? get authCode => _authCode;
   String? _authCode;
 
+  final bool _enableServicePortFallback;
   final bool shouldLogRequests;
 
   Uri get remoteVmServiceUri => _remoteVmServiceUri;

runtime/bin/main_options.cc

@@ -666,6 +666,10 @@ bool Options::ParseArguments(int argc,
         if (implicitly_use_dart_dev && Options::vm_service_auth_disabled()) {
           dart_options->AddArgument("--disable-service-auth-codes");
         }
+        if (implicitly_use_dart_dev &&
+            Options::enable_service_port_fallback()) {
+          dart_options->AddArgument("--enable-service-port-fallback");
+        }
       }
       first_option = false;
     }

sdk/lib/_internal/vm/bin/vmservice_io.dart

@@ -103,6 +103,7 @@ class _DebuggingSession {
         enableDevTools.toString(),
         devToolsBinaries,
         enableLogging.toString(),
+        _enableServicePortFallback.toString(),
       ],
       mode: ProcessStartMode.detachedWithStdio,
     );

sdk/lib/_internal/vm/bin/vmservice_server.dart

@@ -409,11 +409,8 @@ class Server {
       // Already running.
       return this;
     }
-
     // Startup HTTP server.
-    var pollError;
-    var pollStack;
-    Future<bool> poll() async {
+    Future<void> startServer() async {
       try {
         var address;
         var addresses = await InternetAddress.lookup(_ip);
@@ -423,33 +420,22 @@ class Server {
           if (address.type == InternetAddressType.IPv4) break;
         }
         _server = await HttpServer.bind(address, _port);
-        return true;
       } catch (e, st) {
-        pollError = e;
-        pollStack = st;
-        return false;
+        if (_port != 0 && _enableServicePortFallback) {
+          serverPrint('Failed to bind Observatory HTTP server to port $_port. '
+              'Falling back to automatic port selection');
+          _port = 0;
+          await startServer();
+        } else {
+          serverPrint('Could not start Observatory HTTP server:\n'
+              '$e\n$st');
+          _notifyServerState('');
+          onServerAddressChange(null);
+        }
       }
     }
 
-    // poll for the network for ~10 seconds.
-    int attempts = 0;
-    final maxAttempts = 10;
-    while (!await poll()) {
-      attempts++;
-      serverPrint('Observatory server failed to start after $attempts tries');
-      if (attempts > maxAttempts) {
-        serverPrint('Could not start Observatory HTTP server:\n'
-            '$pollError\n$pollStack\n');
-        _notifyServerState('');
-        onServerAddressChange(null);
-        return this;
-      }
-      if (_port != 0 && _enableServicePortFallback && attempts >= 3) {
-        _port = 0;
-        serverPrint('Falling back to automatic port selection');
-      }
-      await Future<void>.delayed(const Duration(seconds: 1));
-    }
+    await startServer();
     if (_service.isExiting) {
       serverPrint('Observatory HTTP server exiting before listening as '
           'vm service has received exit request\n');