From 15330ffbc4ac829fae805b93dad60ba6d7bc7739 Mon Sep 17 00:00:00 2001 From: Michael Goderbauer Date: Wed, 15 Mar 2017 11:28:14 -0700 Subject: [PATCH] Make ProcessSignals portable (#8779) * Make ProcessSignals portable This removes the need to wrap unsupported signals with in `if (!platform.isWindows) ..`. It also allows us to implement a work around for breaking the Windows console when flutter is exited with Ctrl+C. * review comments * adding tests * add license header --- packages/flutter_tools/lib/executable.dart | 1 + packages/flutter_tools/lib/src/base/io.dart | 80 ++++++++++++++++++- .../flutter_tools/lib/src/commands/logs.dart | 11 +-- .../lib/src/resident_runner.dart | 10 +-- .../flutter_tools/test/src/base/io_test.dart | 34 ++++++++ 5 files changed, 120 insertions(+), 16 deletions(-) create mode 100644 packages/flutter_tools/test/src/base/io_test.dart diff --git a/packages/flutter_tools/lib/executable.dart b/packages/flutter_tools/lib/executable.dart index 0f6f5f1c99..e83379be02 100644 --- a/packages/flutter_tools/lib/executable.dart +++ b/packages/flutter_tools/lib/executable.dart @@ -59,6 +59,7 @@ import 'src/usage.dart'; /// /// This function is intended to be used from the `flutter` command line tool. Future main(List args) async { + SigintProcessSignal.instance.init(); final bool verbose = args.contains('-v') || args.contains('--verbose'); final bool help = args.contains('-h') || args.contains('--help') || (args.isNotEmpty && args.first == 'help') || (args.length == 1 && verbose); diff --git a/packages/flutter_tools/lib/src/base/io.dart b/packages/flutter_tools/lib/src/base/io.dart index de6e06e915..15c98d9cc3 100644 --- a/packages/flutter_tools/lib/src/base/io.dart +++ b/packages/flutter_tools/lib/src/base/io.dart @@ -25,10 +25,12 @@ /// about any additional exports that you add to this file, as doing so will /// increase the API surface that we have to test in Flutter tools, and the APIs /// in `dart:io` can sometimes be hard to use in tests. -import 'dart:io' as io show exit; +import 'dart:async'; +import 'dart:io' as io show exit, ProcessSignal; import 'package:meta/meta.dart'; +import 'platform.dart'; import 'process.dart'; export 'dart:io' @@ -55,7 +57,7 @@ export 'dart:io' Process, ProcessException, ProcessResult, - ProcessSignal, + // ProcessSignal NO! Use [ProcessSignal] below. ProcessStartMode, // RandomAccessFile NO! Use `file_system.dart` ServerSocket, @@ -99,3 +101,77 @@ void setExitFunctionForTests([ExitFunction exitFunction]) { void restoreExitFunction() { _exitFunction = _defaultExitFunction; } + +/// A portable version of [io.ProcessSignal]. +/// +/// Listening on signals that don't exist on the current platform is just a +/// no-op. This is in contrast to [io.ProcessSignal], where listening to +/// non-existent signals throws an exception. +class ProcessSignal implements io.ProcessSignal { + @visibleForTesting + const ProcessSignal(this._delegate); + + static const ProcessSignal SIGWINCH = const _PosixProcessSignal._(io.ProcessSignal.SIGWINCH); + static const ProcessSignal SIGTERM = const _PosixProcessSignal._(io.ProcessSignal.SIGTERM); + static const ProcessSignal SIGUSR1 = const _PosixProcessSignal._(io.ProcessSignal.SIGUSR1); + static const ProcessSignal SIGUSR2 = const _PosixProcessSignal._(io.ProcessSignal.SIGUSR2); + + static final ProcessSignal SIGINT = SigintProcessSignal.instance; // ignore: non_constant_identifier_names + + final io.ProcessSignal _delegate; + + @override + Stream watch() { + return _delegate.watch().map((io.ProcessSignal signal) => this); + } + + @override + String toString() => _delegate.toString(); +} + +/// A [ProcessSignal] that is only available on Posix platforms. +/// +/// Listening to a [_PosixProcessSignal] is a no-op on Windows. +class _PosixProcessSignal extends ProcessSignal { + + const _PosixProcessSignal._(io.ProcessSignal wrappedSignal) : super(wrappedSignal); + + @override + Stream watch() { + if (platform.isWindows) + return new Stream.empty(); + return super.watch(); + } +} + +// TODO(goderbauer): remove when https://github.com/dart-lang/sdk/issues/28995 is resolved. +class SigintProcessSignal extends ProcessSignal { + SigintProcessSignal._() : super(io.ProcessSignal.SIGINT); + + static final SigintProcessSignal instance = new SigintProcessSignal._(); + bool _isInitialized = false; + + final StreamController _controller = new StreamController(); + + @override + Stream watch() { + init(); + return _controller.stream; + } + + void init() { + if (!_isInitialized) { + _delegate.watch().listen(_handleSigInt); + _isInitialized = true; + } + } + + void _handleSigInt(io.ProcessSignal signal) { + if (platform.isWindows && !_controller.hasListener) { + // If nobody is listening for SIGINT, we force a clean exit to avoid + // https://github.com/dart-lang/sdk/issues/28995. + exit(0); + } + _controller.add(this); + } +} diff --git a/packages/flutter_tools/lib/src/commands/logs.dart b/packages/flutter_tools/lib/src/commands/logs.dart index 891b70446e..d10eec8fe4 100644 --- a/packages/flutter_tools/lib/src/commands/logs.dart +++ b/packages/flutter_tools/lib/src/commands/logs.dart @@ -6,7 +6,6 @@ import 'dart:async'; import '../base/common.dart'; import '../base/io.dart'; -import '../base/platform.dart'; import '../cache.dart'; import '../device.dart'; import '../globals.dart'; @@ -67,12 +66,10 @@ class LogsCommand extends FlutterCommand { printStatus(''); exitCompleter.complete(0); }); - if (!platform.isWindows) { // https://github.com/dart-lang/sdk/issues/28603 - ProcessSignal.SIGTERM.watch().listen((ProcessSignal signal) { - subscription.cancel(); - exitCompleter.complete(0); - }); - } + ProcessSignal.SIGTERM.watch().listen((ProcessSignal signal) { + subscription.cancel(); + exitCompleter.complete(0); + }); // Wait for the log reader to be finished. final int result = await exitCompleter.future; diff --git a/packages/flutter_tools/lib/src/resident_runner.dart b/packages/flutter_tools/lib/src/resident_runner.dart index 6224f56cef..350214733b 100644 --- a/packages/flutter_tools/lib/src/resident_runner.dart +++ b/packages/flutter_tools/lib/src/resident_runner.dart @@ -13,7 +13,6 @@ import 'base/common.dart'; import 'base/file_system.dart'; import 'base/io.dart'; import 'base/logger.dart'; -import 'base/platform.dart'; import 'base/utils.dart'; import 'build_info.dart'; import 'dart/dependencies.dart'; @@ -163,14 +162,11 @@ abstract class ResidentRunner { void registerSignalHandlers() { assert(stayResident); ProcessSignal.SIGINT.watch().listen(_cleanUpAndExit); - if (!platform.isWindows) // TODO(goderbauer): enable on Windows when https://github.com/dart-lang/sdk/issues/28603 is fixed - ProcessSignal.SIGTERM.watch().listen(_cleanUpAndExit); + ProcessSignal.SIGTERM.watch().listen(_cleanUpAndExit); if (!supportsServiceProtocol || !supportsRestart) return; - if (!platform.isWindows) { - ProcessSignal.SIGUSR1.watch().listen(_handleSignal); - ProcessSignal.SIGUSR2.watch().listen(_handleSignal); - } + ProcessSignal.SIGUSR1.watch().listen(_handleSignal); + ProcessSignal.SIGUSR2.watch().listen(_handleSignal); } Future _cleanUpAndExit(ProcessSignal signal) async { diff --git a/packages/flutter_tools/test/src/base/io_test.dart b/packages/flutter_tools/test/src/base/io_test.dart new file mode 100644 index 0000000000..a817fa38cc --- /dev/null +++ b/packages/flutter_tools/test/src/base/io_test.dart @@ -0,0 +1,34 @@ +// Copyright 2017 The Chromium Authors. 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:async'; +import 'dart:io' as io; + +import 'package:flutter_tools/src/base/io.dart'; +import 'package:mockito/mockito.dart'; +import 'package:test/test.dart'; + +import '../context.dart'; + +void main() { + group('ProcessSignal', () { + + testUsingContext('signals are properly delegated', () async { + final MockIoProcessSignal mockSignal = new MockIoProcessSignal(); + final ProcessSignal signalUnderTest = new ProcessSignal(mockSignal); + final StreamController controller = new StreamController(); + + when(mockSignal.watch()).thenReturn(controller.stream); + controller.add(mockSignal); + + expect(signalUnderTest, await signalUnderTest.watch().first); + }); + + testUsingContext('toString() works', () async { + expect(io.ProcessSignal.SIGINT.toString(), ProcessSignal.SIGINT.toString()); + }); + }); +} + +class MockIoProcessSignal extends Mock implements io.ProcessSignal {} \ No newline at end of file