Make retrieval of an already loaded target interruptible. There is no reason now to forbid it, since Skyframe lookups are interruptible.

--
MOS_MIGRATED_REVID=130429286

5 files changed, 26 insertions, 49 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 8b5eb241b8..fed47ee868 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -56,7 +56,6 @@ import com.google.devtools.build.lib.packages.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
-import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
 import com.google.devtools.build.lib.pkgcache.LoadingResult;
 import com.google.devtools.build.lib.rules.test.CoverageReportActionFactory;
 import com.google.devtools.build.lib.rules.test.CoverageReportActionFactory.CoverageReportActionsWrapper;
@@ -908,10 +907,10 @@ public class BuildView {
       }
 
       @Override
-      protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) {
+      protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses)
+          throws InterruptedException {
         try {
-          return LoadedPackageProvider.getLoadedTarget(
-              skyframeExecutor.getPackageManager(), eventHandler, label);
+          return skyframeExecutor.getPackageManager().getTarget(eventHandler, label);
         } catch (NoSuchThingException e) {
           throw new IllegalStateException(e);
         }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
index ec55f60e8e..2713eca0f4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
@@ -85,7 +85,7 @@ public interface ConfigurationEnvironment {
 
     @Override
     public Target getTarget(final Label label)
-        throws NoSuchPackageException, NoSuchTargetException {
+        throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
       return packageProvider.getLoadedTarget(label);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index 9ce8770270..caa0affcc3 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -250,16 +250,17 @@ public final class BuildTool {
   }
 
   /**
-   * Checks that if this is an environment-restricted build, all top-level targets support
-   * the expected environments.
+   * Checks that if this is an environment-restricted build, all top-level targets support the
+   * expected environments.
    *
    * @param topLevelTargets the build's top-level targets
    * @throws ViewCreationFailedException if constraint enforcement is on, the build declares
-   *     environment-restricted top level configurations, and any top-level target doesn't
-   *     support the expected environments
+   *     environment-restricted top level configurations, and any top-level target doesn't support
+   *     the expected environments
    */
-  private void checkTargetEnvironmentRestrictions(Iterable<ConfiguredTarget> topLevelTargets,
-      LoadedPackageProvider packageManager) throws ViewCreationFailedException {
+  private static void checkTargetEnvironmentRestrictions(
+      Iterable<ConfiguredTarget> topLevelTargets, LoadedPackageProvider packageManager)
+      throws ViewCreationFailedException, InterruptedException {
     for (ConfiguredTarget topLevelTarget : topLevelTargets) {
       BuildConfiguration config = topLevelTarget.getConfiguration();
       if (config == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
index 741e1be26a..a3a288aeef 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
@@ -36,7 +36,6 @@ import com.google.devtools.build.lib.packages.TestTargetUtils;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
-
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
@@ -44,7 +43,6 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
-
 import javax.annotation.Nullable;
 
 /**
@@ -349,8 +347,9 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
     }
   }
 
-  private Set<Target> getTargetsForLabels(
-      LoadedPackageProvider loadedPackageProvider, Collection<Label> labels) {
+  private static Set<Target> getTargetsForLabels(
+      LoadedPackageProvider loadedPackageProvider, Collection<Label> labels)
+      throws InterruptedException {
     Set<Target> result = new HashSet<>();
     for (Label label : labels) {
       try {
@@ -362,9 +361,13 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
     return result;
   }
 
-  private ImmutableSet<Target> filterErrorFreeTargets(EventHandler eventHandler,
-      EventBus eventBus, TransitivePackageLoader pkgLoader, Collection<Target> targetsToLoad,
-      ListMultimap<String, Label> labelsToLoadUnconditionally) throws LoadingFailedException {
+  private ImmutableSet<Target> filterErrorFreeTargets(
+      EventHandler eventHandler,
+      EventBus eventBus,
+      TransitivePackageLoader pkgLoader,
+      Collection<Target> targetsToLoad,
+      ListMultimap<String, Label> labelsToLoadUnconditionally)
+      throws LoadingFailedException, InterruptedException {
     // Error out if any of the labels needed for the configuration could not be loaded.
     Multimap<Label, Label> rootCauses = pkgLoader.getRootCauses();
     for (Map.Entry<String, Label> entry : labelsToLoadUnconditionally.entries()) {
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadedPackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadedPackageProvider.java
index 7bbbb2dea9..09ad33d813 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadedPackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadedPackageProvider.java
@@ -13,16 +13,12 @@
 // limitations under the License.
 package com.google.devtools.build.lib.pkgcache;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.concurrent.Uninterruptibles;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Target;
 
-import java.util.concurrent.Callable;
-
 /**
  * A bridge class that implements the legacy semantics of {@link #getLoadedTarget} using a normal
  * {@link PackageProvider} instance.
@@ -43,35 +39,13 @@ public final class LoadedPackageProvider {
   }
 
   /**
-   * Returns a target if it was recently loaded, i.e., since the most recent cache sync. This
-   * throws an exception if the target was not loaded or not validated, even if it exists in the
+   * Returns a target if it was recently loaded, i.e., since the most recent cache sync. This throws
+   * an exception if the target was not loaded or not validated, even if it exists in the
    * surrounding package. If the surrounding package is in error, still attempts to retrieve the
    * target.
    */
-  public Target getLoadedTarget(Label label) throws NoSuchPackageException, NoSuchTargetException {
-    return getLoadedTarget(packageProvider, eventHandler, label);
-  }
-
-  /**
-   * Uninterruptible method to convert a label into a target using a given package provider and
-   * event handler.
-   */
-  @VisibleForTesting
-  public static Target getLoadedTarget(
-      final PackageProvider packageProvider, final EventHandler eventHandler, final Label label)
-          throws NoSuchPackageException, NoSuchTargetException {
-    try {
-      return Uninterruptibles.callUninterruptibly(new Callable<Target>() {
-        @Override
-        public Target call()
-            throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
-          return packageProvider.getTarget(eventHandler, label);
-        }
-      });
-    } catch (NoSuchPackageException | NoSuchTargetException e) {
-      throw e;
-    } catch (Exception e) {
-      throw new IllegalStateException(e);
-    }
+  public Target getLoadedTarget(Label label)
+      throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
+    return packageProvider.getTarget(eventHandler, label);
   }
 }