Avoid raceconditions in global activate, run and global run

analysis_options.yaml

@@ -31,6 +31,7 @@ linter:
     - sort_pub_dependencies
     - test_types_in_equals
     - throw_in_finally
+    - unawaited_futures
     - unnecessary_lambdas
     - unnecessary_null_aware_assignments
     - unnecessary_parenthesis

lib/src/command/add.dart

@@ -146,9 +146,8 @@ class AddCommand extends PubCommand {
       /// to this new dependency.
       final newRoot = Package.inMemory(updatedPubSpec);
 
-      // TODO(jonasfj): Stop abusing Entrypoint.global for dry-run output
-      await Entrypoint.global(newRoot, entrypoint.lockFile, cache,
-              solveResult: solveResult)
+      await Entrypoint.inMemory(newRoot, cache,
+              solveResult: solveResult, lockFile: entrypoint.lockFile)
           .acquireDependencies(SolveType.get,
               dryRun: true,
               precompile: argResults['precompile'],

lib/src/command/remove.dart

@@ -65,7 +65,7 @@ class RemoveCommand extends PubCommand {
       final newPubspec = _removePackagesFromPubspec(rootPubspec, packages);
       final newRoot = Package.inMemory(newPubspec);
 
-      await Entrypoint.global(newRoot, entrypoint.lockFile, cache)
+      await Entrypoint.inMemory(newRoot, cache, lockFile: entrypoint.lockFile)
           .acquireDependencies(SolveType.get,
               precompile: argResults['precompile'],
               dryRun: true,

lib/src/command/upgrade.dart

@@ -222,11 +222,10 @@ be direct 'dependencies' or 'dev_dependencies', following packages are not:
     if (_dryRun) {
       // Even if it is a dry run, run `acquireDependencies` so that the user
       // gets a report on changes.
-      // TODO(jonasfj): Stop abusing Entrypoint.global for dry-run output
-      await Entrypoint.global(
+      await Entrypoint.inMemory(
         Package.inMemory(resolvablePubspec),
-        entrypoint.lockFile,
         cache,
+        lockFile: entrypoint.lockFile,
         solveResult: solveResult,
       ).acquireDependencies(
         SolveType.upgrade,
@@ -317,10 +316,10 @@ be direct 'dependencies' or 'dev_dependencies', following packages are not:
       // Even if it is a dry run, run `acquireDependencies` so that the user
       // gets a report on changes.
       // TODO(jonasfj): Stop abusing Entrypoint.global for dry-run output
-      await Entrypoint.global(
+      await Entrypoint.inMemory(
         Package.inMemory(nullsafetyPubspec),
-        entrypoint.lockFile,
         cache,
+        lockFile: entrypoint.lockFile,
         solveResult: solveResult,
       ).acquireDependencies(
         SolveType.upgrade,

lib/src/dart.dart

@@ -146,51 +146,80 @@ class AnalyzerErrorGroup implements Exception {
   String toString() => errors.join('\n');
 }
 
-/// Precompiles the Dart executable at [executablePath] to a kernel file at
-/// [outputPath].
+/// Precompiles the Dart executable at [executablePath].
 ///
-/// This file is also cached at [incrementalDillOutputPath] which is used to
-/// initialize the compiler on future runs.
+/// If the compilation succeeds it is saved to a kernel file at [outputPath].
+///
+/// If compilation fails, the output is cached at [incrementalDillOutputPath].
+///
+/// Whichever of [incrementalDillOutputPath] and [outputPath] already exists is
+/// used to initialize the compiler run.
 ///
 /// The [packageConfigPath] should point at the package config file to be used
 /// for `package:` uri resolution.
 ///
 /// The [name] is used to describe the executable in logs and error messages.
 Future<void> precompile({
   required String executablePath,
-  required String incrementalDillOutputPath,
+  required String incrementalDillPath,
   required String name,
   required String outputPath,
   required String packageConfigPath,
 }) async {
   ensureDir(p.dirname(outputPath));
-  ensureDir(p.dirname(incrementalDillOutputPath));
+  ensureDir(p.dirname(incrementalDillPath));
+
   const platformDill = 'lib/_internal/vm_platform_strong.dill';
   final sdkRoot = p.relative(p.dirname(p.dirname(Platform.resolvedExecutable)));
-  var client = await FrontendServerClient.start(
-    executablePath,
-    incrementalDillOutputPath,
-    platformDill,
-    sdkRoot: sdkRoot,
-    packagesJson: packageConfigPath,
-    printIncrementalDependencies: false,
-  );
+  String? tempDir;
+  FrontendServerClient? client;
   try {
-    var result = await client.compile();
+    tempDir = createTempDir(p.dirname(incrementalDillPath), 'tmp');
+    // To avoid potential races we copy the incremental data to a temporary file
+    // for just this compilation.
+    final temporaryIncrementalDill =
+        p.join(tempDir, '${p.basename(incrementalDillPath)}.incremental.dill');
+    try {
+      if (fileExists(incrementalDillPath)) {
+        copyFile(incrementalDillPath, temporaryIncrementalDill);
+      } else if (fileExists(outputPath)) {
+        copyFile(outputPath, temporaryIncrementalDill);
+      }
+    } on FileSystemException {
+      // Not able to copy existing file, compilation will start from scratch.
+    }
+
+    client = await FrontendServerClient.start(
+      executablePath,
+      temporaryIncrementalDill,
+      platformDill,
+      sdkRoot: sdkRoot,
+      packagesJson: packageConfigPath,
+      printIncrementalDependencies: false,
+    );
+    final result = await client.compile();
 
     final highlightedName = log.bold(name);
     if (result?.errorCount == 0) {
       log.message('Built $highlightedName.');
-      await File(incrementalDillOutputPath).copy(outputPath);
+      // By using rename we ensure atomicity. An external observer will either
+      // see the old or the new snapshot.
+      renameFile(temporaryIncrementalDill, outputPath);
     } else {
-      // Don't leave partial results.
-      deleteEntry(outputPath);
+      // By using rename we ensure atomicity. An external observer will either
+      // see the old or the new snapshot.
+      renameFile(temporaryIncrementalDill, incrementalDillPath);
+      // If compilation failed we don't want to leave an incorrect snapshot.
+      tryDeleteEntry(outputPath);
 
       throw ApplicationException(
           log.yellow('Failed to build $highlightedName:\n') +
               (result?.compilerOutputLines.join('\n') ?? ''));
     }
   } finally {
-    client.kill();
+    client?.kill();
+    if (tempDir != null) {
+      tryDeleteEntry(tempDir);
+    }
   }
 }

lib/src/entrypoint.dart

@@ -16,7 +16,6 @@ import 'command_runner.dart';
 import 'dart.dart' as dart;
 import 'exceptions.dart';
 import 'executable.dart';
-import 'http.dart' as http;
 import 'io.dart';
 import 'language_version.dart';
 import 'lock_file.dart';
@@ -73,8 +72,14 @@ final _sdkConstraint = () {
 /// but may be the entrypoint when you're running its tests.
 class Entrypoint {
   /// The root package this entrypoint is associated with.
+  ///
+  /// For a global package, this is the activated package.
   final Package root;
 
+  /// For a global package, this is the directory that the package is installed
+  /// in. Non-global packages have null.
+  final String? globalDir;
+
   /// The system-wide cache which caches packages that need to be fetched over
   /// the network.
   final SystemCache cache;
@@ -83,7 +88,8 @@ class Entrypoint {
   bool get isCached => !root.isInMemory && p.isWithin(cache.rootDir, root.dir);
 
   /// Whether this is an entrypoint for a globally-activated package.
-  final bool isGlobal;
+  // final bool isGlobal;
+  bool get isGlobal => globalDir != null;
 
   /// The lockfile for the entrypoint.
   ///
@@ -123,8 +129,7 @@ class Entrypoint {
   ///
   /// Global packages (except those from path source)
   /// store these in the global cache.
-  String? get _configRoot =>
-      isCached ? p.join(cache.rootDir, 'global_packages', root.name) : root.dir;
+  String? get _configRoot => isCached ? globalDir : root.dir;
 
   /// The path to the entrypoint's ".packages" file.
   ///
@@ -153,11 +158,7 @@ class Entrypoint {
   /// but the configuration is stored at the package itself.
   String get cachePath {
     if (isGlobal) {
-      return p.join(
-        cache.rootDir,
-        'global_packages',
-        root.name,
-      );
+      return globalDir!;
     } else {
       var newPath = root.path('.dart_tool/pub');
       var oldPath = root.path('.pub');
@@ -174,15 +175,25 @@ class Entrypoint {
   String get _incrementalDillsPath => p.join(cachePath, 'incremental');
 
   /// Loads the entrypoint from a package at [rootDir].
-  Entrypoint(String rootDir, this.cache)
-      : root = Package.load(null, rootDir, cache.sources),
-        isGlobal = false;
+  Entrypoint(
+    String rootDir,
+    this.cache,
+  )   : root = Package.load(null, rootDir, cache.sources),
+        globalDir = null;
+
+  Entrypoint.inMemory(this.root, this.cache,
+      {required LockFile? lockFile, SolveResult? solveResult})
+      : _lockFile = lockFile,
+        globalDir = null {
+    if (solveResult != null) {
+      _packageGraph = PackageGraph.fromSolveResult(this, solveResult);
+    }
+  }
 
   /// Creates an entrypoint given package and lockfile objects.
   /// If a SolveResult is already created it can be passed as an optimization.
-  Entrypoint.global(this.root, this._lockFile, this.cache,
-      {SolveResult? solveResult})
-      : isGlobal = true {
+  Entrypoint.global(this.globalDir, this.root, this._lockFile, this.cache,
+      {SolveResult? solveResult}) {
     if (solveResult != null) {
       _packageGraph = PackageGraph.fromSolveResult(this, solveResult);
     }
@@ -203,18 +214,20 @@ class Entrypoint {
 
   /// Writes .packages and .dart_tool/package_config.json
   Future<void> writePackagesFiles() async {
+    final entrypointName = isGlobal ? null : root.name;
     writeTextFile(
         packagesFile,
         lockFile.packagesFile(cache,
-            entrypoint: root.name, relativeFrom: root.dir));
+            entrypoint: entrypointName,
+            relativeFrom: isGlobal ? null : root.dir));
     ensureDir(p.dirname(packageConfigFile));
     writeTextFile(
         packageConfigFile,
         await lockFile.packageConfigFile(cache,
-            entrypoint: root.name,
+            entrypoint: entrypointName,
             entrypointSdkConstraint:
                 root.pubspec.sdkConstraints[sdk.identifier],
-            relativeFrom: root.dir));
+            relativeFrom: isGlobal ? null : root.dir));
   }
 
   /// Gets all dependencies of the [root] package.
@@ -294,7 +307,7 @@ class Entrypoint {
       await result.showReport(type, cache);
     }
     if (!dryRun) {
-      await Future.wait(result.packages.map(_get));
+      await result.downloadCachedPackages(cache);
       _saveLockFile(result);
     }
     if (onlyReportSuccessOrFailure) {
@@ -387,10 +400,11 @@ class Entrypoint {
 
   Future<void> _precompileExecutable(Executable executable) async {
     final package = executable.package;
+
     await dart.precompile(
         executablePath: resolveExecutable(executable),
         outputPath: pathOfExecutable(executable),
-        incrementalDillOutputPath: incrementalDillPathOfExecutable(executable),
+        incrementalDillPath: incrementalDillPathOfExecutable(executable),
         packageConfigPath: packageConfigFile,
         name:
             '$package:${p.basenameWithoutExtension(executable.relativePath)}');
@@ -470,21 +484,6 @@ class Entrypoint {
     }
   }
 
-  /// Makes sure the package at [id] is locally available.
-  ///
-  /// This automatically downloads the package to the system-wide cache as well
-  /// if it requires network access to retrieve (specifically, if the package's
-  /// source is a [CachedSource]).
-  Future<void> _get(PackageId id) async {
-    return await http.withDependencyType(root.dependencyType(id.name),
-        () async {
-      if (id.isRoot) return;
-
-      var source = cache.source(id.source);
-      if (source is CachedSource) await source.downloadToSystemCache(id);
-    });
-  }
-
   /// Throws a [DataError] if the `.dart_tool/package_config.json` file doesn't
   /// exist or if it's out-of-date relative to the lockfile or the pubspec.
   ///