Gracefully handle pubspecs with dependencies using unknown sources.

sdk/lib/_internal/pub/lib/src/solver/version_solver.dart

Index: sdk/lib/_internal/pub/lib/src/solver/version_solver.dart
diff --git a/sdk/lib/_internal/pub/lib/src/solver/version_solver.dart b/sdk/lib/_internal/pub/lib/src/solver/version_solver.dart
index 885acdc55659b5529140d2cdece9e102d6219af5..7034aeeb5374c46a5e136eb7cc62bb9f79b9321f 100644
--- a/sdk/lib/_internal/pub/lib/src/solver/version_solver.dart
+++ b/sdk/lib/_internal/pub/lib/src/solver/version_solver.dart
@@ -14,6 +14,7 @@ import '../pubspec.dart';
 import '../source.dart';
 import '../source_registry.dart';
 import '../version.dart';
+import '../utils.dart';
 import 'backtracking_solver.dart';
 
 /// Attempts to select the best concrete versions for all of the transitive
@@ -70,9 +71,23 @@ class SolveResult {
 /// Used to avoid requesting the same pubspec from the server repeatedly.
 class PubspecCache {
   final SourceRegistry _sources;
+
+  /// The already requested cached version lists.

[nweiz, 2013/05/17 23:55:55]

"already-requested" (also below)

[Bob Nystrom, 2013/05/20 20:06:00]

Done.

   final _versions = new Map<PackageRef, List<PackageId>>();
+
+  /// The already requested cached pubspecs.
   final _pubspecs = new Map<PackageId, Pubspec>();
 
+  /// Pending version list requests that have not completed yet. If multiple
+  /// requests for the same package are made concurrently, this avoids
+  /// duplicate requests.
+  final _pendingVersions = new Map<PackageRef, Future<List<PackageId>>>();
+
+  /// Pending pubspec requests that have not completed yet. If multiple
+  /// requests for the same package are made concurrently, this avoids
+  /// duplicate requests.
+  final _pendingPubspecs = new Map<PackageId, Future<Pubspec>>();

[nweiz, 2013/05/17 23:55:55]

I don't understand why this change is part of this CL.

[Bob Nystrom, 2013/05/20 20:06:00]

The behavior here was always the intended behavior. You shouldn't have multiple
concurrent requests for the same pubspec or version list in flight at the same
time. It was a bug that it didn't do that before.

However, the implementation of the solver, or perhaps the specific tests
themselves happened to never actually kick off a concurrent request and tickle
this bug.

When I moved things around for this patch, that was no longer the case and the
existing bug surfaced. If I recall, this is because _traversePackage calls
Future.wait() to get the lists of versions for each dep in parallel.

But now that I look, that shouldn't cause a concurrent request though because
you shouldn't have the same dep in there twice. I know adding this fixed an
issue, but I'm not sure why/how.

I just tore the code back out and everything still seems fine.

+
   /// The number of times a version list was requested and it wasn't cached and
   /// had to be requested from the source.
   int versionCacheMisses = 0;
@@ -104,11 +119,22 @@ class PubspecCache {
       return new Future<Pubspec>.value(_pubspecs[id]);
     }
 
+    // If we're already waiting on a request, use the same future.
+    if (_pendingPubspecs.containsKey(id)) {
+      pubspecCacheHits++;
+      return _pendingPubspecs[id];
+    }
+
     pubspecCacheMisses++;
-    return id.describe().then((pubspec) {
+
+    var source = _sources[id.source];
+    var future = source.describe(id);
+    _pendingPubspecs[id] = future;
+    return future.then((pubspec) {
       log.solver('requested $id pubspec');
 
       // Cache it.
+      _pendingPubspecs.remove(id);
       _pubspecs[id] = pubspec;
       return pubspec;
     });
@@ -119,21 +145,38 @@ class PubspecCache {
   Pubspec getCachedPubspec(PackageId id) => _pubspecs[id];
 
   /// Gets the list of versions for [package] in descending order.
-  Future<List<PackageId>> getVersions(PackageRef package) {
+  Future<List<PackageId>> getVersions(PackageDep package) {

[nweiz, 2013/05/17 23:55:55]

Why is this taking a PackageDep now? Why is the version constraint relevant?

[Bob Nystrom, 2013/05/20 20:06:00]

That was related to another change I ended up abandoning. Put it back.

+    if (package.isRoot) {
+      throw new StateError("Cannot get versions for root package $package.");
+    }
+
     // See if we have it cached.
-    var versions = _versions[package];
+    var ref = package.toRef();
+    var versions = _versions[ref];
     if (versions != null) {
       versionCacheHits++;
       return new Future.value(versions);
     }
 
+    // If we're already waiting on a request, use the same future.
+    if (_pendingVersions.containsKey(ref)) {
+      pubspecCacheHits++;
+      return _pendingVersions[ref];
+    }
+
     versionCacheMisses++;
-    return package.getVersions().then((ids) {
+
+    var source = _sources[package.source];
+    var future = source.getVersions(package.name, package.description);
+    _pendingVersions[ref] = future;
+    return future.then((versions) {
       // Sort by descending version so we try newer versions first.
-      ids.sort((a, b) => b.version.compareTo(a.version));
+      versions.sort((a, b) => b.compareTo(a));
 
-      log.solver('requested $package version list');
-      _versions[package] = ids;
+      var ids = versions.map((version) => ref.atVersion(version)).toList();
+
+      _pendingVersions.remove(ref);
+      _versions[ref] = ids;
       return ids;

[nweiz, 2013/05/17 23:55:55]

Right now you're returning `ids` for the first future that gets returned for a
[getVersions] call, but if it gets called again and hits [_pendingVersions]
you're returning `versions` instead.

I'm worried that the tests don't catch this.

[Bob Nystrom, 2013/05/20 20:06:00]

Yeah, this code is clearly wrong. Removed.

     });
   }
@@ -153,7 +196,7 @@ class Dependency {
 }
 
 /// Base class for all failures that can occur while trying to resolve versions.
-class SolveFailure implements Exception {
+abstract class SolveFailure implements ApplicationException {
   /// The name of the package whose version could not be solved. Will be `null`
   /// if the failure is not specific to one package.
   final String package;
@@ -270,7 +313,6 @@ class DisjointConstraintException extends SolveFailure {
 /// Exception thrown when two packages with the same name but different sources
 /// are depended upon.
 class SourceMismatchException extends SolveFailure {
-
   SourceMismatchException(String package, Iterable<Dependency> dependencies)
       : super(package, dependencies);
 
@@ -280,6 +322,18 @@ class SourceMismatchException extends SolveFailure {
       "depends on it from source ${dep.source}";
 }
 
+/// Exception thrown when a dependency on an unknown source name is found.
+class UnknownSourceException extends SolveFailure {
+  UnknownSourceException(String package, Iterable<Dependency> dependencies)
+      : super(package, dependencies);
+
+  String toString() {
+    var dep = only(dependencies);
+    return "Package '${dep.depender}' depends on '${dep.dep.name}' from "
+           "unknown source '${dep.dep.source}'.";
+  }
+}
+
 /// Exception thrown when two packages with the same name and source but
 /// different descriptions are depended upon.
 class DescriptionMismatchException extends SolveFailure {