Avoid hanging if we don't use explicit `exit()` (#2704)
* Close the httpClient explicitly.
* Close the signal watching after the process ran
* Don't close the oauth client explicitly
* Use a subProcess to run from the embedding
diff --git a/lib/src/command.dart b/lib/src/command.dart
index 47f4f05..a6ecae2 100644
--- a/lib/src/command.dart
+++ b/lib/src/command.dart
@@ -170,6 +170,8 @@
""");
}
return _chooseExitCode(error);
+ } finally {
+ httpClient.close();
}
}
diff --git a/lib/src/command/global.dart b/lib/src/command/global.dart
index 6b3cde6..a7e8898 100644
--- a/lib/src/command/global.dart
+++ b/lib/src/command/global.dart
@@ -17,10 +17,10 @@
@override
String get docUrl => 'https://dart.dev/tools/pub/cmd/pub-global';
- GlobalCommand() {
+ GlobalCommand({bool alwaysUseSubprocess = false}) {
addSubcommand(GlobalActivateCommand());
addSubcommand(GlobalDeactivateCommand());
addSubcommand(GlobalListCommand());
- addSubcommand(GlobalRunCommand());
+ addSubcommand(GlobalRunCommand(alwaysUseSubprocess: alwaysUseSubprocess));
}
}
diff --git a/lib/src/command/global_run.dart b/lib/src/command/global_run.dart
index da5a015..165997a 100644
--- a/lib/src/command/global_run.dart
+++ b/lib/src/command/global_run.dart
@@ -25,7 +25,9 @@
@override
bool get allowTrailingOptions => false;
- GlobalRunCommand() {
+ final bool alwaysUseSubprocess;
+
+ GlobalRunCommand({this.alwaysUseSubprocess = false}) {
argParser.addFlag('enable-asserts', help: 'Enable assert statements.');
argParser.addFlag('checked', abbr: 'c', hide: true);
argParser.addMultiOption('enable-experiment',
@@ -66,12 +68,16 @@
final vmArgs = vmArgsFromArgResults(argResults);
final globalEntrypoint = await globals.find(package);
- final exitCode = await globals.runExecutable(globalEntrypoint,
- Executable.adaptProgramName(package, executable), args,
- vmArgs: vmArgs,
- enableAsserts: argResults['enable-asserts'] || argResults['checked'],
- recompile: (executable) => log.warningsOnlyUnlessTerminal(
- () => globalEntrypoint.precompileExecutable(executable)));
+ final exitCode = await globals.runExecutable(
+ globalEntrypoint,
+ Executable.adaptProgramName(package, executable),
+ args,
+ vmArgs: vmArgs,
+ enableAsserts: argResults['enable-asserts'] || argResults['checked'],
+ recompile: (executable) => log.warningsOnlyUnlessTerminal(
+ () => globalEntrypoint.precompileExecutable(executable)),
+ alwaysUseSubprocess: alwaysUseSubprocess,
+ );
throw ExitWithException(exitCode);
}
}
diff --git a/lib/src/command/run.dart b/lib/src/command/run.dart
index b106472..567a761 100644
--- a/lib/src/command/run.dart
+++ b/lib/src/command/run.dart
@@ -28,8 +28,12 @@
bool get hidden => deprecated;
final bool deprecated;
+ final bool alwaysUseSubprocess;
- RunCommand({this.deprecated = false}) {
+ RunCommand({
+ this.deprecated = false,
+ this.alwaysUseSubprocess = false,
+ }) {
argParser.addFlag('enable-asserts', help: 'Enable assert statements.');
argParser.addFlag('checked', abbr: 'c', hide: true);
argParser.addMultiOption('enable-experiment',
@@ -93,6 +97,7 @@
recompile: (executable) => log.warningsOnlyUnlessTerminal(
() => entrypoint.precompileExecutable(executable)),
vmArgs: vmArgs,
+ alwaysUseSubprocess: alwaysUseSubprocess,
);
throw ExitWithException(exitCode);
}
@@ -136,10 +141,14 @@
throw ExitWithException(
await runExecutable(
- entrypoint, Executable(package, 'bin/$command.dart'), args,
- vmArgs: vmArgs,
- enableAsserts: argResults['enable-asserts'] || argResults['checked'],
- recompile: entrypoint.precompileExecutable),
+ entrypoint,
+ Executable(package, 'bin/$command.dart'),
+ args,
+ vmArgs: vmArgs,
+ enableAsserts: argResults['enable-asserts'] || argResults['checked'],
+ recompile: entrypoint.precompileExecutable,
+ alwaysUseSubprocess: alwaysUseSubprocess,
+ ),
);
}
}
diff --git a/lib/src/executable.dart b/lib/src/executable.dart
index dfbdf2c..d178362 100644
--- a/lib/src/executable.dart
+++ b/lib/src/executable.dart
@@ -7,8 +7,8 @@
import 'dart:isolate';
import 'package:args/args.dart';
+import 'package:meta/meta.dart';
import 'package:path/path.dart' as p;
-import 'package:pedantic/pedantic.dart';
import 'entrypoint.dart';
import 'exceptions.dart';
@@ -55,7 +55,8 @@
{bool enableAsserts = false,
String packagesFile,
Future<void> Function(Executable) recompile,
- List<String> vmArgs = const []}) async {
+ List<String> vmArgs = const [],
+ @required bool alwaysUseSubprocess}) async {
final package = executable.package;
packagesFile ??= entrypoint.packagesFile;
@@ -124,8 +125,14 @@
var packageConfig = p.absolute(packagesFile);
try {
- return await _runDartProgram(executablePath, args, packageConfig,
- enableAsserts: enableAsserts, vmArgs: vmArgs);
+ return await _runDartProgram(
+ executablePath,
+ args,
+ packageConfig,
+ enableAsserts: enableAsserts,
+ vmArgs: vmArgs,
+ alwaysUseSubprocess: alwaysUseSubprocess,
+ );
} on IsolateSpawnException catch (error) {
if (!useSnapshot ||
!error.message.contains('Invalid kernel binary format version')) {
@@ -134,8 +141,14 @@
log.fine('Precompiled executable is out of date.');
await recompile(executable);
- return _runDartProgram(executablePath, args, packageConfig,
- enableAsserts: enableAsserts, vmArgs: vmArgs);
+ return await _runDartProgram(
+ executablePath,
+ args,
+ packageConfig,
+ enableAsserts: enableAsserts,
+ vmArgs: vmArgs,
+ alwaysUseSubprocess: alwaysUseSubprocess,
+ );
}
}
@@ -149,21 +162,26 @@
/// Passes [vmArgs] to the vm.
///
/// Returns the programs's exit code.
+///
+/// Tries to run the program as an isolate if no special [vmArgs] are given
+/// otherwise starts new vm in a subprocess. If [alwaysUseSubprocess] is `true`
+/// a new process will always be started.
Future<int> _runDartProgram(
String path, List<String> args, String packageConfig,
- {bool enableAsserts, List<String> vmArgs}) async {
+ {bool enableAsserts,
+ List<String> vmArgs,
+ @required bool alwaysUseSubprocess}) async {
path = p.absolute(path);
packageConfig = p.absolute(packageConfig);
// We use Isolate.spawnUri when there are no extra vm-options.
// That provides better signal handling, and possibly faster startup.
- if (vmArgs.isEmpty) {
+ if ((!alwaysUseSubprocess) && vmArgs.isEmpty) {
var argList = args.toList();
- await isolate.runUri(p.toUri(path), argList, null,
+ return await isolate.runUri(p.toUri(path), argList, null,
enableAsserts: enableAsserts,
automaticPackageResolution: packageConfig == null,
packageConfig: p.toUri(packageConfig));
- return exitCode;
} else {
// By ignoring sigint, only the child process will get it when
// they are sent to the current process group. That is what happens when
@@ -184,8 +202,7 @@
// TODO(sigurdm) To handle signals better we would ideally have `exec`
// semantics without `fork` for starting the subprocess.
// https://github.com/dart-lang/sdk/issues/41966.
- unawaited(ProcessSignal.sigint.watch().drain());
-
+ final subscription = ProcessSignal.sigint.watch().listen((e) {});
final process = await Process.start(
Platform.resolvedExecutable,
[
@@ -198,7 +215,9 @@
mode: ProcessStartMode.inheritStdio,
);
- return process.exitCode;
+ final exitCode = await process.exitCode;
+ await subscription.cancel();
+ return exitCode;
}
}
diff --git a/lib/src/global_packages.dart b/lib/src/global_packages.dart
index f96af90..a140721 100644
--- a/lib/src/global_packages.dart
+++ b/lib/src/global_packages.dart
@@ -375,13 +375,21 @@
{bool enableAsserts = false,
String packagesFile,
Future<void> Function(exec.Executable) recompile,
- List<String> vmArgs = const []}) async {
- return await exec.runExecutable(entrypoint, executable, args,
- enableAsserts: enableAsserts,
- packagesFile: packagesFile, recompile: (exectuable) async {
- await recompile(exectuable);
- _refreshBinStubs(entrypoint, executable);
- }, vmArgs: vmArgs);
+ List<String> vmArgs = const [],
+ @required bool alwaysUseSubprocess}) async {
+ return await exec.runExecutable(
+ entrypoint,
+ executable,
+ args,
+ enableAsserts: enableAsserts,
+ packagesFile: packagesFile,
+ recompile: (exectuable) async {
+ await recompile(exectuable);
+ _refreshBinStubs(entrypoint, executable);
+ },
+ vmArgs: vmArgs,
+ alwaysUseSubprocess: alwaysUseSubprocess,
+ );
}
/// Gets the path to the lock file for an activated cached package with
diff --git a/lib/src/http.dart b/lib/src/http.dart
index 0aa43bf..e84467b 100644
--- a/lib/src/http.dart
+++ b/lib/src/http.dart
@@ -142,6 +142,9 @@
return '$name: $value';
}
}
+
+ @override
+ void close() => _inner.close();
}
/// The [_PubHttpClient] wrapped by [httpClient].
@@ -220,6 +223,9 @@
throw PubHttpException(await http.Response.fromStream(streamedResponse));
}
+
+ @override
+ void close() => _inner.close();
}
/// The HTTP client to use for all HTTP requests.
diff --git a/lib/src/isolate.dart b/lib/src/isolate.dart
index 9d0a7aa..58169c7 100644
--- a/lib/src/isolate.dart
+++ b/lib/src/isolate.dart
@@ -14,7 +14,7 @@
/// the [exitCode] variable is set to 255.
///
/// If [buffered] is `true`, this uses [spawnBufferedUri] to spawn the isolate.
-Future runUri(Uri url, List<String> args, Object message,
+Future<int> runUri(Uri url, List<String> args, Object message,
{bool buffered = false,
bool enableAsserts,
bool automaticPackageResolution = false,
@@ -29,12 +29,16 @@
onError: errorPort.sendPort,
onExit: exitPort.sendPort);
- errorPort.listen((list) {
+ final subscription = errorPort.listen((list) {
stderr.writeln('Unhandled exception:');
stderr.writeln(list[0]);
stderr.write(list[1]);
exitCode = 255;
});
-
- await exitPort.first;
+ try {
+ await exitPort.first;
+ } finally {
+ await subscription.cancel();
+ }
+ return exitCode;
}
diff --git a/lib/src/oauth2.dart b/lib/src/oauth2.dart
index fe0cef1..a3e79c4 100644
--- a/lib/src/oauth2.dart
+++ b/lib/src/oauth2.dart
@@ -84,8 +84,10 @@
}
}
-/// Asynchronously passes an OAuth2 [Client] to [fn], and closes the client when
-/// the [Future] returned by [fn] completes.
+/// Asynchronously passes an OAuth2 [Client] to [fn].
+///
+/// Does not close the client, since that would close the shared client. It must
+/// be closed elsewhere.
///
/// This takes care of loading and saving the client's credentials, as well as
/// prompting the user for their authorization. It will also re-authorize and
@@ -93,7 +95,9 @@
Future<T> withClient<T>(SystemCache cache, Future<T> Function(Client) fn) {
return _getClient(cache).then((client) {
return fn(client).whenComplete(() {
- client.close();
+ // TODO(sigurdm): refactor the http subsystem, so we can close [client]
+ // here.
+
// Be sure to save the credentials even when an error happens.
_saveCredentials(cache, client.credentials);
});
diff --git a/lib/src/pub_embeddable_command.dart b/lib/src/pub_embeddable_command.dart
index 2bc6133..6b147fc 100644
--- a/lib/src/pub_embeddable_command.dart
+++ b/lib/src/pub_embeddable_command.dart
@@ -2,8 +2,8 @@
// 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 'command.dart' show PubCommand;
import 'command.dart';
+import 'command.dart' show PubCommand;
import 'command/add.dart';
import 'command/build.dart';
import 'command/cache.dart';
@@ -49,12 +49,12 @@
addSubcommand(CacheCommand());
addSubcommand(DepsCommand());
addSubcommand(DowngradeCommand());
- addSubcommand(GlobalCommand());
+ addSubcommand(GlobalCommand(alwaysUseSubprocess: true));
addSubcommand(GetCommand());
addSubcommand(LishCommand());
addSubcommand(OutdatedCommand());
addSubcommand(RemoveCommand());
- addSubcommand(RunCommand(deprecated: true));
+ addSubcommand(RunCommand(deprecated: true, alwaysUseSubprocess: true));
addSubcommand(UpgradeCommand());
addSubcommand(UploaderCommand());
addSubcommand(LoginCommand());
diff --git a/tool/test-bin/pub_command_runner.dart b/tool/test-bin/pub_command_runner.dart
index 056f105..ec32015 100644
--- a/tool/test-bin/pub_command_runner.dart
+++ b/tool/test-bin/pub_command_runner.dart
@@ -2,9 +2,9 @@
import 'package:args/args.dart';
import 'package:args/command_runner.dart';
-import 'package:pub/src/pub_embeddable_command.dart';
-import 'package:pub/src/log.dart' as log;
import 'package:pub/src/exit_codes.dart' as exit_codes;
+import 'package:pub/src/log.dart' as log;
+import 'package:pub/src/pub_embeddable_command.dart';
class Runner extends CommandRunner<int> {
ArgResults _options;