Allow reverse dependency lookups on Skyframe graph to throw Interupte…

…dException

--
MOS_MIGRATED_REVID=132999234

src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java

@@ -64,12 +64,14 @@ public Set<SkyKey> setValue(SkyValue value, Version version) throws InterruptedE
   }
 
   @Override
-  public DependencyState addReverseDepAndCheckIfDone(@Nullable SkyKey reverseDep) {
+  public DependencyState addReverseDepAndCheckIfDone(@Nullable SkyKey reverseDep)
+      throws InterruptedException {
     return getDelegate().addReverseDepAndCheckIfDone(reverseDep);
   }
 
   @Override
-  public DependencyState checkIfDoneForDirtyReverseDep(SkyKey reverseDep) {
+  public DependencyState checkIfDoneForDirtyReverseDep(SkyKey reverseDep)
+      throws InterruptedException {
     return getDelegate().checkIfDoneForDirtyReverseDep(reverseDep);
   }
 
@@ -159,7 +161,7 @@ public Iterable<SkyKey> getDirectDeps() throws InterruptedException {
   }
 
   @Override
-  public void removeReverseDep(SkyKey reverseDep) {
+  public void removeReverseDep(SkyKey reverseDep) throws InterruptedException {
     getDelegate().removeReverseDep(reverseDep);
   }
 
@@ -169,7 +171,7 @@ public void removeInProgressReverseDep(SkyKey reverseDep) {
   }
 
   @Override
-  public Iterable<SkyKey> getReverseDeps() {
+  public Iterable<SkyKey> getReverseDeps() throws InterruptedException {
     return getDelegate().getReverseDeps();
   }
 

src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java

@@ -282,7 +282,16 @@ public void run() {
 
                 if (traverseGraph) {
                   // Propagate deletion upwards.
-                  visit(entry.getReverseDeps(), InvalidationType.DELETED);
+                  try {
+                    visit(entry.getReverseDeps(), InvalidationType.DELETED);
+                  } catch (InterruptedException e) {
+                    throw new IllegalStateException(
+                        "Deletion cannot happen on a graph that may have blocking operations: "
+                            + key
+                            + ", "
+                            + entry,
+                        e);
+                  }
 
                   // Unregister this node as an rdep from its direct deps, since reverse dep
                   // edges cannot point to non-existent nodes. To know whether the child has this
@@ -318,7 +327,17 @@ public void run() {
                     NodeEntry dep = directDepEntry.getValue();
                     if (dep != null) {
                       if (dep.isDone() || !signalingDeps.contains(directDepEntry.getKey())) {
-                        dep.removeReverseDep(key);
+                        try {
+                          dep.removeReverseDep(key);
+                        } catch (InterruptedException e) {
+                          throw new IllegalStateException(
+                              "Deletion cannot happen on a graph that may have blocking "
+                                  + "operations: "
+                                  + key
+                                  + ", "
+                                  + entry,
+                              e);
+                        }
                       } else {
                         // This step is not strictly necessary, since all in-progress nodes are
                         // deleted during graph cleaning, which happens in a single

src/main/java/com/google/devtools/build/skyframe/NodeEntry.java

@@ -98,7 +98,7 @@ enum DirtyState {
 
   /** Removes a reverse dependency. */
   @ThreadSafe
-  void removeReverseDep(SkyKey reverseDep);
+  void removeReverseDep(SkyKey reverseDep) throws InterruptedException;
 
   /**
    * Removes a reverse dependency.
@@ -114,7 +114,7 @@ enum DirtyState {
    * check-then-act race; {@link #removeReverseDep} may fail for a key that is returned here.
    */
   @ThreadSafe
-  Iterable<SkyKey> getReverseDeps();
+  Iterable<SkyKey> getReverseDeps() throws InterruptedException;
 
   /**
    * Returns raw {@link SkyValue} stored in this entry, which may include metadata associated with
@@ -166,9 +166,9 @@ enum DirtyState {
 
   /**
    * Queries if the node is done and adds the given key as a reverse dependency. The return code
-   * indicates whether a) the node is done, b) the reverse dependency is the first one, so the
-   * node needs to be scheduled, or c) the reverse dependency was added, and the node does not
-   * need to be scheduled.
+   * indicates whether a) the node is done, b) the reverse dependency is the first one, so the node
+   * needs to be scheduled, or c) the reverse dependency was added, and the node does not need to be
+   * scheduled.
    *
    * <p>This method <b>must</b> be called before any processing of the entry. This encourages
    * callers to check that the entry is ready to be processed.
@@ -179,14 +179,15 @@ enum DirtyState {
    * However, a may complete first, in which case b has to re-schedule itself. Also see {@link
    * #setValue}.
    *
-   * <p>If the parameter is {@code null}, then no reverse dependency is added, but we still check
-   * if the node needs to be scheduled.
+   * <p>If the parameter is {@code null}, then no reverse dependency is added, but we still check if
+   * the node needs to be scheduled.
    *
    * <p>If {@code reverseDep} is a rebuilding dirty entry that was already a reverse dep of this
    * entry, then {@link #checkIfDoneForDirtyReverseDep} must be called instead.
    */
   @ThreadSafe
-  DependencyState addReverseDepAndCheckIfDone(@Nullable SkyKey reverseDep);
+  DependencyState addReverseDepAndCheckIfDone(@Nullable SkyKey reverseDep)
+      throws InterruptedException;
 
   /**
    * Similar to {@link #addReverseDepAndCheckIfDone}, except that {@param reverseDep} must already
@@ -196,7 +197,7 @@ enum DirtyState {
    * node for evaluation if needed.
    */
   @ThreadSafe
-  DependencyState checkIfDoneForDirtyReverseDep(SkyKey reverseDep);
+  DependencyState checkIfDoneForDirtyReverseDep(SkyKey reverseDep) throws InterruptedException;
 
   /**
    * Tell this node that one of its dependencies is now done. Callers must check the return value,

src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java

@@ -207,7 +207,8 @@ private void enqueueChild(
         NodeEntry entry,
         SkyKey child,
         NodeEntry childEntry,
-        boolean depAlreadyExists) {
+        boolean depAlreadyExists)
+        throws InterruptedException {
       Preconditions.checkState(!entry.isDone(), "%s %s", skyKey, entry);
       DependencyState dependencyState =
           depAlreadyExists
@@ -595,13 +596,16 @@ private String prepareCrashMessage(SkyKey skyKey, Iterable<SkyKey> reverseDeps)
    * its requested deps are done. However, that means we're assuming the SkyFunction would throw
    * that same error if all of its requested deps were done. Unfortunately, there is no way to
    * enforce that condition.
+   *
+   * @throws InterruptedException
    */
   private static void registerNewlyDiscoveredDepsForDoneEntry(
       SkyKey skyKey,
       NodeEntry entry,
       Map<SkyKey, ? extends NodeEntry> newlyRequestedDepMap,
       Set<SkyKey> oldDeps,
-      SkyFunctionEnvironment env) {
+      SkyFunctionEnvironment env)
+      throws InterruptedException {
     Set<SkyKey> unfinishedDeps = new HashSet<>();
     for (SkyKey dep : env.getNewlyRequestedDeps()) {
       if (!isDoneForBuild(newlyRequestedDepMap.get(dep))) {

src/test/java/com/google/devtools/build/skyframe/DeterministicHelper.java

@@ -135,7 +135,7 @@ private DeterministicValueEntry(SkyKey myKey, ThinNodeEntry delegate) {
     }
 
     @Override
-    public synchronized Collection<SkyKey> getReverseDeps() {
+    public synchronized Collection<SkyKey> getReverseDeps() throws InterruptedException {
       TreeSet<SkyKey> result = new TreeSet<>(ALPHABETICAL_SKYKEY_COMPARATOR);
       Iterables.addAll(result, super.getReverseDeps());
       return result;

src/test/java/com/google/devtools/build/skyframe/GraphConcurrencyTest.java

@@ -193,14 +193,14 @@ public void run() {
                     .isNotEqualTo(DependencyState.DONE);
                 waitForAddedRdep.countDown();
                 waitForSetValue.await(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+                for (int k = chunkSize; k <= numIterations; k++) {
+                  entry.removeReverseDep(key("rdep" + j));
+                  entry.addReverseDepAndCheckIfDone(key("rdep" + j));
+                  entry.getReverseDeps();
+                }
               } catch (InterruptedException e) {
                 fail("Test failed: " + e.toString());
               }
-              for (int k = chunkSize; k <= numIterations; k++) {
-                entry.removeReverseDep(key("rdep" + j));
-                entry.addReverseDepAndCheckIfDone(key("rdep" + j));
-                entry.getReverseDeps();
-              }
             }
           };
       pool.execute(wrapper.wrap(r));
@@ -245,6 +245,7 @@ public void testAddingInflightNodes() throws Exception {
           final Iterable<SkyKey> keys = ImmutableList.of(key1, key2);
           Runnable r =
               new Runnable() {
+                @Override
                 public void run() {
                   for (SkyKey key : keys) {
                     NodeEntry entry = null;
@@ -268,15 +269,15 @@ public void run() {
                     NodeEntry entry = entries.get(key);
                     // {@code entry.addReverseDepAndCheckIfDone(null)} should return
                     // NEEDS_SCHEDULING at most once.
-                    if (startEvaluation(entry).equals(DependencyState.NEEDS_SCHEDULING)) {
-                      assertTrue(valuesSet.add(key));
-                      // Set to done.
-                      try {
+                    try {
+                      if (startEvaluation(entry).equals(DependencyState.NEEDS_SCHEDULING)) {
+                        assertTrue(valuesSet.add(key));
+                        // Set to done.
                         entry.setValue(new StringValue("bar" + keyNum), startingVersion);
-                      } catch (InterruptedException e) {
-                        throw new IllegalStateException(key + ", " + entry, e);
+                        assertThat(entry.isDone()).isTrue();
                       }
-                      assertThat(entry.isDone()).isTrue();
+                    } catch (InterruptedException e) {
+                      throw new IllegalStateException(key + ", " + entry, e);
                     }
                   }
                   // This shouldn't cause any problems from the other threads.
@@ -360,16 +361,13 @@ public void run() {
               }
               try {
                 entry.markDirty(true);
-              } catch (InterruptedException e) {
-                throw new IllegalStateException(keyNum + ", " + entry, e);
-              }
-              // Make some changes, like adding a dep and rdep.
-              entry.addReverseDepAndCheckIfDone(key("rdep"));
-              entry.markRebuilding();
-              addTemporaryDirectDep(entry, key("dep"));
-              entry.signalDep();
-              // Move node from dirty back to done.
-              try {
+
+                // Make some changes, like adding a dep and rdep.
+                entry.addReverseDepAndCheckIfDone(key("rdep"));
+                entry.markRebuilding();
+                addTemporaryDirectDep(entry, key("dep"));
+                entry.signalDep();
+
                 entry.setValue(new StringValue("bar" + keyNum), getNextVersion(startingVersion));
               } catch (InterruptedException e) {
                 throw new IllegalStateException(keyNum + ", " + entry, e);
@@ -461,7 +459,7 @@ public void run() {
     }
   }
 
-  private static DependencyState startEvaluation(NodeEntry entry) {
+  private static DependencyState startEvaluation(NodeEntry entry) throws InterruptedException {
     return entry.addReverseDepAndCheckIfDone(null);
   }
 

src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java

@@ -56,7 +56,7 @@ private static SkyKey key(String name) {
 
   @Test
   public void createEntry() {
-    NodeEntry entry = new InMemoryNodeEntry();
+    InMemoryNodeEntry entry = new InMemoryNodeEntry();
     entry.addReverseDepAndCheckIfDone(null); // Start evaluation.
     assertFalse(entry.isDone());
     assertTrue(entry.isReady());
@@ -148,7 +148,7 @@ public void crashOnNullErrorAndValue() throws InterruptedException {
 
   @Test
   public void crashOnTooManySignals() {
-    NodeEntry entry = new InMemoryNodeEntry();
+    InMemoryNodeEntry entry = new InMemoryNodeEntry();
     entry.addReverseDepAndCheckIfDone(null); // Start evaluation.
     try {
       entry.signalDep();

src/test/java/com/google/devtools/build/skyframe/NodeEntrySubject.java

@@ -21,7 +21,6 @@
 import com.google.common.truth.FailureStrategy;
 import com.google.common.truth.IterableSubject;
 import com.google.common.truth.Subject;
-
 import javax.annotation.Nullable;
 
 /**
@@ -43,7 +42,7 @@ IterableSubject hasTemporaryDirectDepsThat() {
   }
 
   ComparableSubject<?, NodeEntry.DependencyState> addReverseDepAndCheckIfDone(
-      @Nullable SkyKey reverseDep) {
+      @Nullable SkyKey reverseDep) throws InterruptedException {
     return assertThat(getSubject().addReverseDepAndCheckIfDone(reverseDep))
         .named(detail("AddReverseDepAndCheckIfDone"));
   }

src/test/java/com/google/devtools/build/skyframe/NotifyingHelper.java

@@ -216,15 +216,16 @@ protected ThinNodeEntry getThinDelegate() {
     }
 
     @Override
-    public DependencyState addReverseDepAndCheckIfDone(SkyKey reverseDep) {
+    public DependencyState addReverseDepAndCheckIfDone(SkyKey reverseDep)
+        throws InterruptedException {
       graphListener.accept(myKey, EventType.ADD_REVERSE_DEP, Order.BEFORE, reverseDep);
       DependencyState result = super.addReverseDepAndCheckIfDone(reverseDep);
       graphListener.accept(myKey, EventType.ADD_REVERSE_DEP, Order.AFTER, reverseDep);
       return result;
     }
 
     @Override
-    public void removeReverseDep(SkyKey reverseDep) {
+    public void removeReverseDep(SkyKey reverseDep) throws InterruptedException {
       graphListener.accept(myKey, EventType.REMOVE_REVERSE_DEP, Order.BEFORE, reverseDep);
       super.removeReverseDep(reverseDep);
       graphListener.accept(myKey, EventType.REMOVE_REVERSE_DEP, Order.AFTER, reverseDep);
@@ -293,7 +294,8 @@ public SkyValue getValueMaybeWithMetadata() throws InterruptedException {
     }
 
     @Override
-    public DependencyState checkIfDoneForDirtyReverseDep(SkyKey reverseDep) {
+    public DependencyState checkIfDoneForDirtyReverseDep(SkyKey reverseDep)
+        throws InterruptedException {
       graphListener.accept(myKey, EventType.CHECK_IF_DONE, Order.BEFORE, reverseDep);
       return super.checkIfDoneForDirtyReverseDep(reverseDep);
     }