From b8018a88ccd2c2d2fe959276a2fcc121f71c2b99 Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Fri, 30 Aug 2024 17:51:52 +0000 Subject: [PATCH] [ Service ] Fix race in startup / shutdown code Fixes https://github.com/dart-lang/sdk/issues/56606 TEST=regress_56606_test.dart CoreLibraryReviewExempt: VM service only change Change-Id: Ie3a96d277547ac8ef4bf45ba700ad311d5997817 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/383060 Reviewed-by: Derek Xu Auto-Submit: Ben Konyi Commit-Queue: Ben Konyi --- pkg/dartdev/test/regress_56606_test.dart | 53 ++++++++++++++++ sdk/lib/_internal/vm/bin/vmservice_io.dart | 2 +- .../_internal/vm/bin/vmservice_server.dart | 63 +++++++++---------- sdk/lib/vmservice/vmservice.dart | 3 +- 4 files changed, 84 insertions(+), 37 deletions(-) create mode 100644 pkg/dartdev/test/regress_56606_test.dart diff --git a/pkg/dartdev/test/regress_56606_test.dart b/pkg/dartdev/test/regress_56606_test.dart new file mode 100644 index 00000000000..0bf77cc6f8f --- /dev/null +++ b/pkg/dartdev/test/regress_56606_test.dart @@ -0,0 +1,53 @@ +// 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 'dart:io'; + +import 'package:path/path.dart' as path; +import 'package:test/test.dart'; +import 'utils.dart'; + +const numRuns = 10; + +final smokeTestScript = r''' +void main() { + print('Smoke test!'); +} +'''; + +void main() { + late final String script; + late final TestProject p; + + setUp(() { + p = project(mainSrc: smokeTestScript); + script = path.join(p.dirPath, p.relativeFilePath); + }); + + tearDown(() async { + await p.dispose(); + }); + + test( + 'Regression test for https://github.com/dart-lang/sdk/issues/56606', + () async { + // Tests for a race condition in the VM service startup / shutdown logic + // that previously caused crashes when the VM service was enabled for + // programs with short lifespans. + for (int i = 1; i <= numRuns; ++i) { + if (i % 5 == 0) { + print('Done [$i/$numRuns]'); + } + final result = await Process.run( + Platform.executable, + ['--enable-vm-service', script], + environment: {'BOT': '1'}, + ); + expect(result.stderr, isEmpty); + expect(result.stdout, contains('Smoke test!')); + expect(result.exitCode, 0); + } + }, + ); +} diff --git a/sdk/lib/_internal/vm/bin/vmservice_io.dart b/sdk/lib/_internal/vm/bin/vmservice_io.dart index b5a8e2d569f..f7e107ea512 100644 --- a/sdk/lib/_internal/vm/bin/vmservice_io.dart +++ b/sdk/lib/_internal/vm/bin/vmservice_io.dart @@ -96,7 +96,7 @@ Future cleanupCallback() async { final localServer = server; if (localServer != null) { try { - await localServer.cleanup(true); + await localServer.shutdown(true); } catch (e, st) { print('Error in vm-service shutdown: $e\n$st\n'); } diff --git a/sdk/lib/_internal/vm/bin/vmservice_server.dart b/sdk/lib/_internal/vm/bin/vmservice_server.dart index c71ea274f30..13c61d96340 100644 --- a/sdk/lib/_internal/vm/bin/vmservice_server.dart +++ b/sdk/lib/_internal/vm/bin/vmservice_server.dart @@ -216,10 +216,7 @@ class _DebuggingSession { return true; } - void shutdown() { - print('Shutting down DDS!\n${StackTrace.current}'); - _process!.kill(); - } + void shutdown() => _process!.kill(); Process? _process; } @@ -635,22 +632,15 @@ class Server { } } - Future cleanup(bool force) { - final serverLocal = _server; - if (serverLocal == null) { - return Future.value(); - } - if (Platform.isFuchsia) { - // Remove the file with the port number. - final tmp = Directory.systemTemp.path; - final path = '$tmp/dart.services/${serverLocal.port}'; - serverPrint('Deleting $path'); - File(path)..deleteSync(); - } - return serverLocal.close(force: force); + void _cleanupFuchsiaState(int port) { + // Remove the file with the port number. + final tmp = Directory.systemTemp.path; + final path = '$tmp/dart.services/$port'; + serverPrint('Deleting $path'); + File(path).deleteSync(); } - Future shutdown(bool forced) async { + Future shutdown(bool forced) async { // If start is pending, wait for it to complete. if (_startingCompleter != null) { if (!_startingCompleter!.isCompleted) { @@ -658,32 +648,37 @@ class Server { } } - if (_server == null) { + final server = _server; + if (server == null) { // Not started. - return Future.value(this); + return; } - // Shutdown HTTP server and subscription. - Uri oldServerAddress = serverAddress!; - return cleanup(forced).then((_) { - serverPrint('Dart VM service no longer listening on $oldServerAddress'); - _server = null; - _startingCompleter = null; + if (Platform.isFuchsia) { + _cleanupFuchsiaState(server.port); + } + + final address = serverAddress!; + + try { + // Shutdown HTTP server and subscription. + await server.close(force: forced); + if (!_service.isExiting) { + // Only print this message if the service has been toggled off, not + // when the VM is exiting. + serverPrint('Dart VM service no longer listening on $address'); + } + } catch (e, st) { + serverPrint('Could not shutdown Dart VM service HTTP server:\n$e\n$st\n'); + } finally { _ddsInstance?.shutdown(); _ddsInstance = null; - _running = false; - _notifyServerState(''); - onServerAddressChange(null); - return this; - }).catchError((e, st) { _server = null; _startingCompleter = null; - serverPrint('Could not shutdown Dart VM service HTTP server:\n$e\n$st\n'); _running = false; _notifyServerState(''); onServerAddressChange(null); - return this; - }); + } } } diff --git a/sdk/lib/vmservice/vmservice.dart b/sdk/lib/vmservice/vmservice.dart index b7b11bcbf85..4a531f21436 100644 --- a/sdk/lib/vmservice/vmservice.dart +++ b/sdk/lib/vmservice/vmservice.dart @@ -476,12 +476,11 @@ class VMService extends MessageRouter { // Close receive ports. isolateControlPort.close(); scriptLoadPort.close(); - await clearState(); final cleanup = VMServiceEmbedderHooks.cleanup; if (cleanup != null) { await cleanup(); } - + await clearState(); // Notify the VM that we have exited. _onExit(); }