From 03f7be96d5912066a3ffb727de81eab67ebc6dcd Mon Sep 17 00:00:00 2001 From: Gary Roumanis Date: Tue, 23 Apr 2019 13:10:13 -0700 Subject: [PATCH 1/3] Retry build_daemon state file reads --- build_daemon/CHANGELOG.md | 4 +++ build_daemon/lib/client.dart | 9 +++++-- build_daemon/lib/constants.dart | 7 +++++- build_daemon/lib/src/daemon.dart | 14 +++++++++-- build_daemon/pubspec.yaml | 2 +- build_daemon/test/daemon_test.dart | 40 ++++++++++++++++++++++-------- 6 files changed, 60 insertions(+), 16 deletions(-) diff --git a/build_daemon/CHANGELOG.md b/build_daemon/CHANGELOG.md index e730882066..229d7616e9 100644 --- a/build_daemon/CHANGELOG.md +++ b/build_daemon/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.6.0 + +- Add retry logic to the state file helpers `runningVersion` and `currentOptions`. + ## 0.5.1 - Support shutting down the daemon with a notification. diff --git a/build_daemon/lib/client.dart b/build_daemon/lib/client.dart index e2076a0840..7f49e41253 100644 --- a/build_daemon/lib/client.dart +++ b/build_daemon/lib/client.dart @@ -17,8 +17,13 @@ import 'data/build_target_request.dart'; import 'data/serializers.dart'; import 'data/server_log.dart'; -int _existingPort(String workingDirectory) { +Future _existingPort(String workingDirectory) async { var portFile = File(portFilePath(workingDirectory)); + var retryCount = 0; + while (!portFile.existsSync() && retryCount < maxRetries) { + await Future.delayed(Duration(milliseconds: 100)); + retryCount++; + } if (!portFile.existsSync()) throw MissingPortFile(); return int.parse(portFile.readAsStringSync()); } @@ -129,7 +134,7 @@ class BuildDaemonClient { await _handleDaemonStartup(process, logHandler); return BuildDaemonClient._( - _existingPort(workingDirectory), daemonSerializers, logHandler); + await _existingPort(workingDirectory), daemonSerializers, logHandler); } } diff --git a/build_daemon/lib/constants.dart b/build_daemon/lib/constants.dart index a0643e04de..db6c996b5a 100644 --- a/build_daemon/lib/constants.dart +++ b/build_daemon/lib/constants.dart @@ -11,7 +11,7 @@ const versionSkew = 'DIFFERENT RUNNING VERSION'; const optionsSkew = 'DIFFERENT OPTIONS'; // TODO(grouma) - use pubspec version when this is open sourced. -const currentVersion = '5.0.0'; +const currentVersion = '6.0.0'; var _username = Platform.environment['USER'] ?? ''; String daemonWorkspace(String workingDirectory) { @@ -24,6 +24,11 @@ String daemonWorkspace(String workingDirectory) { return p.joinAll(segments); } +/// The number of times to retry reading the various state files. +/// +/// This prevents the likelihood of race conditions. +const maxRetries = 50; + /// Used to ensure that only one instance of this daemon is running at a time. String lockFilePath(String workingDirectory) => p.join(daemonWorkspace(workingDirectory), '.dart_build_lock'); diff --git a/build_daemon/lib/src/daemon.dart b/build_daemon/lib/src/daemon.dart index 28dcfb00e6..9ce17224ef 100644 --- a/build_daemon/lib/src/daemon.dart +++ b/build_daemon/lib/src/daemon.dart @@ -17,8 +17,13 @@ import 'server.dart'; /// Returns the current version of the running build daemon. /// /// Null if one isn't running. -String runningVersion(String workingDirectory) { +Future runningVersion(String workingDirectory) async { var versionFile = File(versionFilePath(workingDirectory)); + var retryCount = 0; + while (!versionFile.existsSync() && retryCount < maxRetries) { + await Future.delayed(Duration(milliseconds: 100)); + retryCount++; + } if (!versionFile.existsSync()) return null; return versionFile.readAsStringSync(); } @@ -26,8 +31,13 @@ String runningVersion(String workingDirectory) { /// Returns the current options of the running build daemon. /// /// Null if one isn't running. -Set currentOptions(String workingDirectory) { +Future> currentOptions(String workingDirectory) async { var optionsFile = File(optionsFilePath(workingDirectory)); + var retryCount = 0; + while (!optionsFile.existsSync() && retryCount < maxRetries) { + await Future.delayed(Duration(milliseconds: 100)); + retryCount++; + } if (!optionsFile.existsSync()) return Set(); return optionsFile.readAsLinesSync().toSet(); } diff --git a/build_daemon/pubspec.yaml b/build_daemon/pubspec.yaml index 0f41c0336f..f937905a39 100644 --- a/build_daemon/pubspec.yaml +++ b/build_daemon/pubspec.yaml @@ -1,5 +1,5 @@ name: build_daemon -version: 0.5.1 +version: 0.6.0 description: A daemon for running Dart builds. author: Dart Team homepage: https://github.com/dart-lang/build/tree/master/build_daemon diff --git a/build_daemon/test/daemon_test.dart b/build_daemon/test/daemon_test.dart index 2ca7e98e77..c6934ed076 100644 --- a/build_daemon/test/daemon_test.dart +++ b/build_daemon/test/daemon_test.dart @@ -83,19 +83,29 @@ void main() { expect(await getOutput(daemonTwo), 'RUNNING'); }); + test('can start two daemons at the same time', () async { + var workspace = uuid.v1(); + testWorkspaces.add(workspace); + var daemonOne = await _runDaemon(workspace); + var daemonTwo = await _runDaemon(workspace); + expect([await getOutput(daemonOne), await getOutput(daemonTwo)], + containsAll(['RUNNING', 'ALREADY RUNNING'])); + testDaemons.addAll([daemonOne, daemonTwo]); + }); + test('logs the version when running', () async { var workspace = uuid.v1(); testWorkspaces.add(workspace); var daemon = await _runDaemon(workspace); testDaemons.add(daemon); expect(await getOutput(daemon), 'RUNNING'); - expect(runningVersion(workspace), currentVersion); + expect(await runningVersion(workspace), currentVersion); }); test('does not set the current version if not running', () async { var workspace = uuid.v1(); testWorkspaces.add(workspace); - expect(runningVersion(workspace), null); + expect(await runningVersion(workspace), null); }); test('logs the options when running', () async { @@ -104,13 +114,13 @@ void main() { var daemon = await _runDaemon(workspace); testDaemons.add(daemon); expect(await getOutput(daemon), 'RUNNING'); - expect(currentOptions(workspace).contains('foo'), isTrue); + expect((await currentOptions(workspace)).contains('foo'), isTrue); }); test('does not log the options if not running', () async { var workspace = uuid.v1(); testWorkspaces.add(workspace); - expect(currentOptions(workspace).isEmpty, isTrue); + expect((await currentOptions(workspace)).isEmpty, isTrue); }); test('cleans up after itself', () async { @@ -128,27 +138,37 @@ void main() { }); } -Future getOutput(Process daemon) async { - return await daemon.stdout - .transform(utf8.decoder) - .transform(const LineSplitter()) - .firstWhere((line) => line == 'RUNNING' || line == 'ALREADY RUNNING'); -} +Future getOutput(Process daemon) async => + await daemon.stdout + .transform(utf8.decoder) + .transform(const LineSplitter()) + .firstWhere((line) => line == 'RUNNING' || line == 'ALREADY RUNNING', + orElse: () => null) ?? + (await daemon.stderr + .transform(utf8.decoder) + .transform(const LineSplitter()) + .toList()) + .join('\n'); Future _runDaemon(var workspace, {int timeout = 30}) async { await d.file('test.dart', ''' import 'package:build_daemon/src/daemon.dart'; import 'package:build_daemon/daemon_builder.dart'; + import 'package:build_daemon/client.dart'; main() async { var daemon = Daemon('$workspace'); if (daemon.tryGetLock()) { var options = ['foo'].toSet(); var timeout = Duration(seconds: $timeout); + await Future.delayed(Duration(seconds: 1)); await daemon.start(options, DaemonBuilder(), Stream.empty(), timeout: timeout); print('RUNNING'); } else { + // Mimic the behavior of actual daemon implementations. + var version = await runningVersion('$workspace'); + if(version != '$currentVersion') throw VersionSkew(); print('ALREADY RUNNING'); } } From b9741d82b02d8c156d7db931c80a05d078797fc4 Mon Sep 17 00:00:00 2001 From: Gary Roumanis Date: Tue, 23 Apr 2019 14:48:16 -0700 Subject: [PATCH 2/3] update build runner to use the latest --- build_runner/CHANGELOG.md | 4 ++++ build_runner/lib/src/entrypoint/daemon.dart | 4 ++-- build_runner/pubspec.yaml | 6 ++++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/build_runner/CHANGELOG.md b/build_runner/CHANGELOG.md index 31418b8da1..910ebf8dd1 100644 --- a/build_runner/CHANGELOG.md +++ b/build_runner/CHANGELOG.md @@ -1,3 +1,7 @@ +## 1.3.5 + +- Use the latest `build_daemon`. + ## 1.3.4 - Use the latest `build_config`. diff --git a/build_runner/lib/src/entrypoint/daemon.dart b/build_runner/lib/src/entrypoint/daemon.dart index 41f954c92c..c918ca4e39 100644 --- a/build_runner/lib/src/entrypoint/daemon.dart +++ b/build_runner/lib/src/entrypoint/daemon.dart @@ -32,8 +32,8 @@ class DaemonCommand extends BuildRunnerCommand { var daemon = Daemon(workingDirectory); var requestedOptions = argResults.arguments.toSet(); if (!daemon.tryGetLock()) { - var runningOptions = currentOptions(workingDirectory); - var version = runningVersion(workingDirectory); + var runningOptions = await currentOptions(workingDirectory); + var version = await runningVersion(workingDirectory); if (version != currentVersion) { stdout ..writeln('Running Version: $version') diff --git a/build_runner/pubspec.yaml b/build_runner/pubspec.yaml index a50d03fb08..8cde2afdd2 100644 --- a/build_runner/pubspec.yaml +++ b/build_runner/pubspec.yaml @@ -1,5 +1,5 @@ name: build_runner -version: 1.3.4 +version: 1.3.5 description: Tools to write binaries that run builders. author: Dart Team homepage: https://github.com/dart-lang/build/tree/master/build_runner @@ -12,7 +12,7 @@ dependencies: async: ">=1.13.3 <3.0.0" build: ">=1.0.0 <1.2.0" build_config: ">=0.4.0 <0.4.1" - build_daemon: ^0.5.0 + build_daemon: ^0.6.0 build_resolvers: "^1.0.0" build_runner_core: ^3.0.0 code_builder: ">2.3.0 <4.0.0" @@ -59,3 +59,5 @@ dependency_overrides: path: ../build_config build_runner_core: path: ../build_runner_core + build_daemon: + path: ../build_daemon From 69ba343c04687fac2ef5a27049f44565e0f42d06 Mon Sep 17 00:00:00 2001 From: Gary Roumanis Date: Wed, 24 Apr 2019 13:19:01 -0700 Subject: [PATCH 3/3] respond to review --- analysis_options.yaml | 2 +- build_daemon/lib/client.dart | 8 ++--- build_daemon/lib/constants.dart | 5 --- build_daemon/lib/src/daemon.dart | 15 ++------- build_daemon/lib/src/file_wait.dart | 24 +++++++++++++++ build_daemon/test/daemon_test.dart | 47 +++++++++++++++++------------ 6 files changed, 57 insertions(+), 44 deletions(-) create mode 100644 build_daemon/lib/src/file_wait.dart diff --git a/analysis_options.yaml b/analysis_options.yaml index b3ce339dc0..ec52f93c66 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -49,7 +49,7 @@ linter: #- implementation_imports - invariant_booleans - iterable_contains_unrelated_type - - join_return_with_assignment + #- join_return_with_assignment - library_names - library_prefixes - list_remove_unrelated_type diff --git a/build_daemon/lib/client.dart b/build_daemon/lib/client.dart index 7f49e41253..7843a0ccda 100644 --- a/build_daemon/lib/client.dart +++ b/build_daemon/lib/client.dart @@ -16,15 +16,11 @@ import 'data/build_status.dart'; import 'data/build_target_request.dart'; import 'data/serializers.dart'; import 'data/server_log.dart'; +import 'src/file_wait.dart'; Future _existingPort(String workingDirectory) async { var portFile = File(portFilePath(workingDirectory)); - var retryCount = 0; - while (!portFile.existsSync() && retryCount < maxRetries) { - await Future.delayed(Duration(milliseconds: 100)); - retryCount++; - } - if (!portFile.existsSync()) throw MissingPortFile(); + if (!await waitForFile(portFile)) throw MissingPortFile(); return int.parse(portFile.readAsStringSync()); } diff --git a/build_daemon/lib/constants.dart b/build_daemon/lib/constants.dart index db6c996b5a..711fa99b72 100644 --- a/build_daemon/lib/constants.dart +++ b/build_daemon/lib/constants.dart @@ -24,11 +24,6 @@ String daemonWorkspace(String workingDirectory) { return p.joinAll(segments); } -/// The number of times to retry reading the various state files. -/// -/// This prevents the likelihood of race conditions. -const maxRetries = 50; - /// Used to ensure that only one instance of this daemon is running at a time. String lockFilePath(String workingDirectory) => p.join(daemonWorkspace(workingDirectory), '.dart_build_lock'); diff --git a/build_daemon/lib/src/daemon.dart b/build_daemon/lib/src/daemon.dart index 9ce17224ef..86c548697f 100644 --- a/build_daemon/lib/src/daemon.dart +++ b/build_daemon/lib/src/daemon.dart @@ -12,6 +12,7 @@ import 'package:watcher/watcher.dart'; import '../constants.dart'; import '../daemon_builder.dart'; import '../data/build_target.dart'; +import 'file_wait.dart'; import 'server.dart'; /// Returns the current version of the running build daemon. @@ -19,12 +20,7 @@ import 'server.dart'; /// Null if one isn't running. Future runningVersion(String workingDirectory) async { var versionFile = File(versionFilePath(workingDirectory)); - var retryCount = 0; - while (!versionFile.existsSync() && retryCount < maxRetries) { - await Future.delayed(Duration(milliseconds: 100)); - retryCount++; - } - if (!versionFile.existsSync()) return null; + if (!await waitForFile(versionFile)) return null; return versionFile.readAsStringSync(); } @@ -33,12 +29,7 @@ Future runningVersion(String workingDirectory) async { /// Null if one isn't running. Future> currentOptions(String workingDirectory) async { var optionsFile = File(optionsFilePath(workingDirectory)); - var retryCount = 0; - while (!optionsFile.existsSync() && retryCount < maxRetries) { - await Future.delayed(Duration(milliseconds: 100)); - retryCount++; - } - if (!optionsFile.existsSync()) return Set(); + if (!await waitForFile(optionsFile)) return Set(); return optionsFile.readAsLinesSync().toSet(); } diff --git a/build_daemon/lib/src/file_wait.dart b/build_daemon/lib/src/file_wait.dart new file mode 100644 index 0000000000..f318103c39 --- /dev/null +++ b/build_daemon/lib/src/file_wait.dart @@ -0,0 +1,24 @@ +// Copyright (c) 2019, 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:async'; +import 'dart:io'; + +const _readDelay = Duration(milliseconds: 100); +const _maxWait = Duration(seconds: 5); + +/// Returns true if the file exists. +/// +/// If the file does not exist it keeps retrying for a period of time. +/// Returns false if the file never becomes available. +/// +/// This reduces the likelihood of race conditions. +Future waitForFile(File file) async { + final end = DateTime.now().add(_maxWait); + while (!DateTime.now().isAfter(end)) { + if (file.existsSync()) return true; + await Future.delayed(_readDelay); + } + return file.existsSync(); +} diff --git a/build_daemon/test/daemon_test.dart b/build_daemon/test/daemon_test.dart index c6934ed076..63c4ed3195 100644 --- a/build_daemon/test/daemon_test.dart +++ b/build_daemon/test/daemon_test.dart @@ -50,7 +50,7 @@ void main() { var workspace = uuid.v1(); var daemon = await _runDaemon(workspace); testDaemons.add(daemon); - expect(await getOutput(daemon), 'RUNNING'); + expect(await _statusOf(daemon), 'RUNNING'); }); test('shuts down if no client connects', () async { @@ -65,10 +65,10 @@ void main() { var workspace = uuid.v1(); testWorkspaces.add(workspace); var daemonOne = await _runDaemon(workspace); - expect(await getOutput(daemonOne), 'RUNNING'); + expect(await _statusOf(daemonOne), 'RUNNING'); var daemonTwo = await _runDaemon(workspace); testDaemons.addAll([daemonOne, daemonTwo]); - expect(await getOutput(daemonTwo), 'ALREADY RUNNING'); + expect(await _statusOf(daemonTwo), 'ALREADY RUNNING'); }); test('can run if another daemon is running in a different workspace', @@ -77,10 +77,10 @@ void main() { var workspace2 = uuid.v1(); testWorkspaces.addAll([workspace1, workspace2]); var daemonOne = await _runDaemon(workspace1); - expect(await getOutput(daemonOne), 'RUNNING'); + expect(await _statusOf(daemonOne), 'RUNNING'); var daemonTwo = await _runDaemon(workspace2); testDaemons.addAll([daemonOne, daemonTwo]); - expect(await getOutput(daemonTwo), 'RUNNING'); + expect(await _statusOf(daemonTwo), 'RUNNING'); }); test('can start two daemons at the same time', () async { @@ -88,7 +88,7 @@ void main() { testWorkspaces.add(workspace); var daemonOne = await _runDaemon(workspace); var daemonTwo = await _runDaemon(workspace); - expect([await getOutput(daemonOne), await getOutput(daemonTwo)], + expect([await _statusOf(daemonOne), await _statusOf(daemonTwo)], containsAll(['RUNNING', 'ALREADY RUNNING'])); testDaemons.addAll([daemonOne, daemonTwo]); }); @@ -98,7 +98,7 @@ void main() { testWorkspaces.add(workspace); var daemon = await _runDaemon(workspace); testDaemons.add(daemon); - expect(await getOutput(daemon), 'RUNNING'); + expect(await _statusOf(daemon), 'RUNNING'); expect(await runningVersion(workspace), currentVersion); }); @@ -113,7 +113,7 @@ void main() { testWorkspaces.add(workspace); var daemon = await _runDaemon(workspace); testDaemons.add(daemon); - expect(await getOutput(daemon), 'RUNNING'); + expect(await _statusOf(daemon), 'RUNNING'); expect((await currentOptions(workspace)).contains('foo'), isTrue); }); @@ -128,7 +128,7 @@ void main() { testWorkspaces.add(workspace); var daemon = await _runDaemon(workspace); // Wait for the daemon to be running before checking the workspace exits. - expect(await getOutput(daemon), 'RUNNING'); + expect(await _statusOf(daemon), 'RUNNING'); expect(Directory(daemonWorkspace(workspace)).existsSync(), isTrue); // Daemon expects sigint twice before quitting. daemon..kill(ProcessSignal.sigint)..kill(ProcessSignal.sigint); @@ -138,17 +138,22 @@ void main() { }); } -Future getOutput(Process daemon) async => - await daemon.stdout - .transform(utf8.decoder) - .transform(const LineSplitter()) - .firstWhere((line) => line == 'RUNNING' || line == 'ALREADY RUNNING', - orElse: () => null) ?? - (await daemon.stderr - .transform(utf8.decoder) - .transform(const LineSplitter()) - .toList()) - .join('\n'); +/// Returns the daemon status. +/// +/// If the status is null, the stderr ir returned. +Future _statusOf(Process daemon) async { + var status = await daemon.stdout + .transform(utf8.decoder) + .transform(const LineSplitter()) + .firstWhere((line) => line == 'RUNNING' || line == 'ALREADY RUNNING', + orElse: () => null); + status ??= (await daemon.stderr + .transform(utf8.decoder) + .transform(const LineSplitter()) + .toList()) + .join('\n'); + return status; +} Future _runDaemon(var workspace, {int timeout = 30}) async { await d.file('test.dart', ''' @@ -161,6 +166,8 @@ Future _runDaemon(var workspace, {int timeout = 30}) async { if (daemon.tryGetLock()) { var options = ['foo'].toSet(); var timeout = Duration(seconds: $timeout); + // Real implementations of the daemon usually non-trivial set up time + // before calling start. await Future.delayed(Duration(seconds: 1)); await daemon.start(options, DaemonBuilder(), Stream.empty(), timeout: timeout);