Fix race condition with delete events for generated files, fixes #175

build_runner/CHANGELOG.md

@@ -1,3 +1,15 @@
+## 0.3.0
+
+### Bug Fixes
+
+- Fixed a race condition bug [175](https://github.com/dart-lang/build/issues/175)
+  that could cause invalid output errors.
+
+### Breaking Changes
+
+- `RunnerAssetWriter` now requires an additional field, `onDelete` which is a
+  callback that must be called synchronously within `delete`.
+
 ## 0.2.0
 
 Add support for the new bytes apis in `build`.

build_runner/lib/src/asset/file_based.dart

@@ -83,6 +83,8 @@ AssetId _fileToAssetId(File file, PackageNode packageNode) {
 /// files to disk.
 class FileBasedAssetWriter implements RunnerAssetWriter {
   final PackageGraph packageGraph;
+  @override
+  OnDelete onDelete;
 
   FileBasedAssetWriter(this.packageGraph);
 
@@ -102,13 +104,16 @@ class FileBasedAssetWriter implements RunnerAssetWriter {
   }
 
   @override
-  Future delete(AssetId id) async {
+  Future delete(AssetId id) {
     assert(id.package == packageGraph.root.name);
+    if (onDelete != null) onDelete(id);
 
     var file = _fileFor(id, packageGraph);
-    if (await file.exists()) {
-      await file.delete();
-    }
+    return () async {
+      if (await file.exists()) {
+        await file.delete();
+      }
+    }();
   }
 }
 

build_runner/lib/src/asset/writer.dart

@@ -2,6 +2,12 @@ import 'dart:async';
 
 import 'package:build/build.dart';
 
+typedef void OnDelete(AssetId id);
+
 abstract class RunnerAssetWriter implements AssetWriter {
+  /// Called synchronously whenever an asset is deleted.
+  OnDelete onDelete;
+
+  /// Deletes an asset, and calls [onDelete] synchronously.
   Future delete(AssetId id);
 }

build_runner/lib/src/generate/watch_impl.dart

@@ -62,12 +62,21 @@ class WatchImpl {
 
   Completer _onTerminatedCompleter = new Completer();
   Future get onTerminated => _onTerminatedCompleter.future;
+
+  /// Pending expected delete events from the writer.
+  final Set<AssetId> _expectedDeletes = new Set<AssetId>(); 
 
   WatchImpl(BuildOptions options, PhaseGroup phaseGroup)
       : _directoryWatcherFactory = options.directoryWatcherFactory,
         _debounceDelay = options.debounceDelay,
         _packageGraph = options.packageGraph,
-        _buildImpl = new BuildImpl(options, phaseGroup);
+        _buildImpl = new BuildImpl(options, phaseGroup) {
+    var existingOnDelete = options.writer.onDelete;
+    options.writer.onDelete = (id) {
+      _expectedDeletes.add(id);
+      if (existingOnDelete != null) existingOnDelete(id);
+    };
+  }
 
   /// Completes after the current build is done, and stops further builds from
   /// happening.
@@ -140,6 +149,7 @@ class WatchImpl {
         if (_shouldSkipInput(input, changeType)) return;
         updatedInputsCopy[input] = changeType;
       });
+      _expectedDeletes.clear();
       updatedInputs.clear();
       if (updatedInputsCopy.isEmpty && !force) {
         return;
@@ -210,6 +220,14 @@ class WatchImpl {
           _logger.finest(
               'Got ${e.type} event for path $relativePath from ${watcher.path}');
           var id = new AssetId(package.name, relativePath);
+
+          // Short circuit for expected deletes (these are a part of the build).
+          if (_expectedDeletes.contains(id)) {
+            // Remove the expected delete so real deletes later on work.
+            _expectedDeletes.remove(id);
+            return;
+          }
+
           var node = _assetGraph.get(id);
           // Short circuit for deletes of nodes that aren't in the graph.
           if (e.type == ChangeType.REMOVE && node == null) return;

build_runner/pubspec.yaml

@@ -1,5 +1,5 @@
 name: build_runner
-version: 0.2.0
+version: 0.3.0
 description: Tools to write binaries that run builders.
 author: Dart Team <misc@dartlang.org>
 homepage: https://github.com/dart-lang/build

build_runner/test/common/in_memory_writer.dart

@@ -9,8 +9,12 @@ import 'package:build_test/build_test.dart';
 
 class InMemoryRunnerAssetWriter extends InMemoryAssetWriter
     implements RunnerAssetWriter {
+  @override
+  OnDelete onDelete;
+
   @override
   Future delete(AssetId id) async {
+    if (onDelete != null) onDelete(id); 
     assets.remove(id);
   }
 }

build_runner/test/generate/watch_test.dart

@@ -94,7 +94,8 @@ void main() {
           'a|web/b.txt.copy': 'b',
         }, result.outputs, writer);
 
-        await writer.delete(makeAssetId('a|web/a.txt'));
+        // Don't call writer.delete, that has side effects.
+        writer.assets.remove(makeAssetId('a|web/a.txt'));
         FakeWatcher.notifyWatchers(new WatchEvent(
             ChangeType.REMOVE, path.absolute('a', 'web', 'a.txt')));
 
@@ -125,7 +126,8 @@ void main() {
         await writer.writeAsString(makeAssetId('a|web/c.txt'), 'c');
         FakeWatcher.notifyWatchers(
             new WatchEvent(ChangeType.ADD, path.absolute('a', 'web', 'c.txt')));
-        await writer.delete(makeAssetId('a|web/a.txt'));
+        // Don't call writer.delete, that has side effects.
+        writer.assets.remove(makeAssetId('a|web/a.txt'));
         FakeWatcher.notifyWatchers(new WatchEvent(
             ChangeType.REMOVE, path.absolute('a', 'web', 'a.txt')));
 
@@ -298,7 +300,9 @@ void main() {
           'a|web/b.txt.copy.copy': 'b'
         }, result.outputs, writer);
 
-        await writer.delete(makeAssetId('a|web/a.txt'));
+        // Don't call writer.delete, that has side effects.
+        writer.assets.remove(makeAssetId('a|web/a.txt'));
+
         FakeWatcher.notifyWatchers(new WatchEvent(
             ChangeType.REMOVE, path.absolute('a', 'web', 'a.txt')));
 
@@ -331,7 +335,8 @@ void main() {
           'a|web/a.txt.copy.copy': 'a',
         }, result.outputs, writer);
 
-        await writer.delete(makeAssetId('a|web/a.txt.copy'));
+        // Don't call writer.delete, that has side effects.
+        writer.assets.remove(makeAssetId('a|web/a.txt.copy'));
         FakeWatcher.notifyWatchers(new WatchEvent(
             ChangeType.REMOVE, path.absolute('a', 'web', 'a.txt.copy')));