Merge pull request #14 from Workiva/fix_hostname_bug

rmconsole4-wk · web-flow · commit 56d402aef4b7 · 2021-08-12T10:34:15.000-05:00
CPLAT-15165 Fix bug when using `--hostname` option
diff --git a/.github/workflows/dart_ci.yml b/.github/workflows/dart_ci.yml
@@ -0,0 +1,36 @@
+name: Dart CI
+
+on:
+  push:
+    branches:
+      - 'master'
+      - 'test_consume_*'
+  pull_request:
+    branches:
+      - '*'
+
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        sdk: [ stable, dev ]
+    steps:
+      - uses: actions/checkout@v2
+      - uses: dart-lang/setup-dart@v0.2
+        with:
+          sdk: ${{ matrix.sdk }}
+
+      - name: Install dependencies
+        run: dart pub get
+
+      - name: Formatting
+        run: dart format --output=none --set-exit-if-changed .
+        if: always() && ${{ matrix.sdk }} == 'stable'
+
+      - name: Analysis
+        run: dart analyze
+
+      - name: Tests
+        run: dart test
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,4 +1,8 @@
+## 0.1.1
+
+- Fix bug when using `--hostname` that would cause the build daemon to fail to
+start.
+
 ## 0.1.0
 
 - Initial release!
-
diff --git a/Dockerfile b/Dockerfile
@@ -1,7 +1,5 @@
-FROM google/dart:2.3
-RUN apt-get update -qq && rm -rf /var/lib/apt/lists/*
+FROM google/dart:2.13
 WORKDIR /build/
 ADD pubspec.yaml .
 RUN pub get
-ARG BUILD_ARTIFACTS_BUILD=/build/pubspec.lock
 FROM scratch
diff --git a/dart_test.yaml b/dart_test.yaml
@@ -1,3 +1,4 @@
 concurrency: 1
 reporter: expanded
-retry: 2
+retry: 2
+timeout: 60s
diff --git a/lib/src/command_runner.dart b/lib/src/command_runner.dart
@@ -26,7 +26,7 @@ class WebdevProxy extends CommandRunner<int> {
   WebdevProxy()
       : super('webdev_proxy',
             'A simple dart proxy for `webdev serve` (uses the `shelf_proxy` package).') {
-    addCommand(new ServeCommand());
+    addCommand(ServeCommand());
     argParser.addFlag(verboseFlag, abbr: 'v', help: 'Enable verbose output.');
   }
 
diff --git a/lib/src/command_utils.dart b/lib/src/command_utils.dart
@@ -19,7 +19,7 @@ import 'package:args/args.dart';
 void assertNoPositionalArgsBeforeSeparator(
   String command,
   ArgResults argResults,
-  void usageException(String msg),
+  void Function(String msg) usageException,
 ) {
   if (argResults.rest.isEmpty) {
     return;
diff --git a/lib/src/logging.dart b/lib/src/logging.dart
@@ -50,7 +50,7 @@ StringBuffer colorLog(LogRecord record, {bool verbose}) {
   }
 
   if (record.stackTrace != null && verbose) {
-    final trace = new Trace.from(record.stackTrace).terse;
+    final trace = Trace.from(record.stackTrace).terse;
     lines.add(trace);
   }
 
@@ -100,7 +100,7 @@ String humanReadable(Duration duration) {
 Future<T> logTimedAsync<T>(
   Logger logger,
   String description,
-  Future<T> action(), {
+  Future<T> Function() action, {
   Level level = Level.INFO,
 }) async {
   final watch = Stopwatch()..start();
@@ -118,7 +118,7 @@ Future<T> logTimedAsync<T>(
 T logTimedSync<T>(
   Logger logger,
   String description,
-  T action(), {
+  T Function() action, {
   Level level = Level.INFO,
 }) {
   final watch = Stopwatch()..start();
diff --git a/lib/src/port_utils.dart b/lib/src/port_utils.dart
@@ -14,11 +14,20 @@
 
 import 'dart:io';
 
-/// Returns a Future that completes with an open port that should be available
-/// for binding.
-Future<int> findAndReleaseOpenPort() async {
-  final socket = await ServerSocket.bind('localhost', 0);
-  final port = socket.port;
+/// Returns a port that is probably, but not definitely, not in use.
+///
+/// This has a built-in race condition: another process may bind this port at
+/// any time after this call has returned.
+Future<int> findUnusedPort() async {
+  int port;
+  ServerSocket socket;
+  try {
+    socket =
+        await ServerSocket.bind(InternetAddress.loopbackIPv6, 0, v6Only: true);
+  } on SocketException {
+    socket = await ServerSocket.bind(InternetAddress.loopbackIPv4, 0);
+  }
+  port = socket.port;
   await socket.close();
   return port;
 }
diff --git a/lib/src/serve_command.dart b/lib/src/serve_command.dart
@@ -137,22 +137,23 @@ class ServeCommand extends Command<int> {
     });
 
     // Parse the hostname to serve each dir on (defaults to 0.0.0.0)
-    final hostname = parseHostname(argResults.rest);
+    final hostnameResults = parseHostname(argResults.rest);
+    final hostname = hostnameResults.hostname;
+    final remainingArgs = hostnameResults.remainingArgs;
 
     // Parse the directory:port mappings that will be used by the proxy servers.
     // Each proxy will be mapped to a `webdev serve` instance on another port.
     final portsToServeByDir = parseDirectoryArgs(argResults.rest);
 
     // Find open ports for each of the directories to be served by webdev.
     final portsToProxyByDir = {
-      for (final dir in portsToServeByDir.keys)
-        dir: await findAndReleaseOpenPort()
+      for (final dir in portsToServeByDir.keys) dir: await findUnusedPort()
     };
 
     // Start the underlying `webdev serve` process.
     webdevServer = await WebdevServer.start([
-      ...argResults.rest,
       if (hostname != 'localhost') '--hostname=$hostname',
+      ...remainingArgs,
       for (final dir in portsToServeByDir.keys)
         '$dir:${portsToProxyByDir[dir]}',
     ]);
diff --git a/lib/src/webdev_arg_utils.dart b/lib/src/webdev_arg_utils.dart
@@ -53,23 +53,34 @@ Map<String, int> parseDirectoryArgs(List<String> args) {
 /// [args] only if it is specified.
 ///
 /// Otherwise, returns a default of `'localhost'`.
-String parseHostname(List<String> args) {
+ParseHostnameResults parseHostname(List<String> args) {
+  var hostname = 'localhost';
+  final remainingArgs = <String>[];
+  var skipNext = false;
   for (var i = 0; i < args.length; i++) {
-    if (!args[i].startsWith('--hostname')) {
+    if (skipNext) {
+      skipNext = false;
       continue;
-    }
-    if (args[i].contains('=')) {
+    } else if (!args[i].startsWith('--hostname')) {
+      remainingArgs.add(args[i]);
+    } else if (args[i].contains('=')) {
       // --hostname=<value>
-      return args[i].split('=')[1];
-    }
-    if (i + 1 < args.length && !args[i + 1].startsWith('-')) {
+      hostname = args[i].split('=')[1];
+    } else if (i + 1 < args.length && !args[i + 1].startsWith('-')) {
       // --hostname <value>
-      return args[i + 1];
+      hostname = args[i + 1];
+      skipNext = true;
     }
   }
-  return 'localhost';
+  return ParseHostnameResults(hostname, remainingArgs);
 
   // TODO: Use when webdev `--hostname=any` support is released
   // HttpMultiServer supports `any` as a more flexible localhost.
   // return 'any';
 }
+
+class ParseHostnameResults {
+  final String hostname;
+  final List<String> remainingArgs;
+  ParseHostnameResults(this.hostname, this.remainingArgs);
+}
diff --git a/lib/src/webdev_proc_utils.dart b/lib/src/webdev_proc_utils.dart
@@ -21,7 +21,7 @@ import 'package:pub_semver/pub_semver.dart';
 
 /// The range of `webdev` versions with which this `webdev_proxy` package is
 /// compatible.
-final webdevCompatibility = new VersionConstraint.parse('>=1.0.1 <3.0.0');
+final webdevCompatibility = VersionConstraint.parse('>=1.0.1 <3.0.0');
 
 @visibleForTesting
 ProcessResult cachedWebdevVersionResult;
diff --git a/pubspec.yaml b/pubspec.yaml
@@ -7,10 +7,6 @@ description: >
   A simple HTTP proxy for webdev's serve command. Supports apps that use HTML5
   routing by rewriting 404s to the root index.
 
-authors:
-  - Workiva Client Platform Team <clientplatform@workiva.com>
-  - Evan Weible <evan.weible@workiva.com>
-
 environment:
   sdk: ">=2.3.2 <3.0.0"
 
@@ -21,7 +17,7 @@ dependencies:
   http_multi_server: ^2.1.0
   io: ^0.3.3
   logging: ^0.11.3+2
-  meta: ^1.1.7
+  meta: ">=1.1.7 <1.7.0" # pin to avoid issue in 1.7.0
   pedantic: ^1.7.0
   pub_semver: ^1.4.2
   shelf: ^0.7.2
@@ -34,9 +30,9 @@ dev_dependencies:
   build_web_compilers: '>=1.2.2 <3.0.0'
 
   shelf_static: ^0.2.8
-  sse: ^2.0.2
+  sse: ^3.6.1
   test: ^1.6.3
-  webdriver: ^2.0.0
+  webdriver: ^3.0.0
 
 executables:
   webdev_proxy:
diff --git a/test/chromedriver_utils.dart b/test/chromedriver_utils.dart
@@ -0,0 +1,57 @@
+import 'dart:convert';
+import 'dart:io';
+
+import 'package:test/test.dart';
+import 'package:webdev_proxy/src/port_utils.dart';
+import 'package:webdriver/io.dart' as wd;
+
+const chromeDriverPort = 4444;
+const chromeDriverUrlBase = 'wd/hub';
+
+Future<void> startChromeDriver() async {
+  try {
+    final chromeDriver = await Process.start(
+        'chromedriver', ['--port=4444', '--url-base=wd/hub']);
+    addTearDown(chromeDriver.kill);
+
+    // On windows this takes a while to boot up, wait for the first line
+    // of stdout as a signal that it is ready.
+    final stdOutLines = chromeDriver.stdout
+        .transform(utf8.decoder)
+        .transform(LineSplitter())
+        .asBroadcastStream();
+
+    final stdErrLines = chromeDriver.stderr
+        .transform(utf8.decoder)
+        .transform(LineSplitter())
+        .asBroadcastStream();
+
+    stdOutLines.listen((line) => print('ChromeDriver stdout: $line'));
+    stdErrLines.listen((line) => print('ChromeDriver stderr: $line'));
+
+    await stdOutLines.first;
+  } catch (e) {
+    throw StateError(
+        'Could not start ChromeDriver. Is it installed?\nError: $e');
+  }
+}
+
+Future<wd.WebDriver> createWebDriver() async {
+  final debugPort = await findUnusedPort();
+  final capabilities = wd.Capabilities.chrome
+    ..addAll({
+      wd.Capabilities.chromeOptions: {
+        'args': [
+          'remote-debugging-port=$debugPort',
+          '--headless',
+        ]
+      }
+    });
+  final webDriver = await wd.createDriver(
+      spec: wd.WebDriverSpec.JsonWire,
+      desired: capabilities,
+      uri: Uri.parse(
+          'http://127.0.0.1:$chromeDriverPort/$chromeDriverUrlBase/'));
+  addTearDown(webDriver.quit);
+  return webDriver;
+}
diff --git a/test/port_utils_test.dart b/test/port_utils_test.dart
@@ -20,9 +20,9 @@ import 'package:test/test.dart';
 import 'package:webdev_proxy/src/port_utils.dart';
 
 void main() {
-  group('findAndReleaseOpenPort()', () {
+  group('findUnusedPort()', () {
     test('should return an open port', () async {
-      final port = await findAndReleaseOpenPort();
+      final port = await findUnusedPort();
       ServerSocket socket;
       try {
         socket = await ServerSocket.bind('localhost', port);
@@ -34,8 +34,8 @@ void main() {
     });
 
     test('should return distinct ports when called multiple times', () async {
-      final port1 = await findAndReleaseOpenPort();
-      final port2 = await findAndReleaseOpenPort();
+      final port1 = await findUnusedPort();
+      final port2 = await findUnusedPort();
       expect(port1, isNot(port2), reason: 'Ports should be distinct.');
     });
   });
diff --git a/test/proxy_server_test.dart b/test/proxy_server_test.dart
diff --git a/test/sse_proxy_handler_test.dart b/test/sse_proxy_handler_test.dart
diff --git a/test/webdev_arg_utils_test.dart b/test/webdev_arg_utils_test.dart
diff --git a/test/webdev_proxy_server_test.dart b/test/webdev_proxy_server_test.dart
diff --git a/test/webdev_server_test.dart b/test/webdev_server_test.dart

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ class WebdevProxy extends CommandRunner<int> {`
`26`	`26`	`WebdevProxy()`
`27`	`27`	`: super('webdev_proxy',`
`28`	`28`	'A simple dart proxy for `webdev serve` (uses the `shelf_proxy` package).') {
`29`		`- addCommand(new ServeCommand());`
	`29`	`+ addCommand(ServeCommand());`
`30`	`30`	`argParser.addFlag(verboseFlag, abbr: 'v', help: 'Enable verbose output.');`
`31`	`31`	`}`
`32`	`32`