From bb54224bd9eb7b1974ae4b1bea329dcb0c6689bc Mon Sep 17 00:00:00 2001
From: Googler <noreply@google.com>
Date: Tue, 13 Aug 2019 06:38:07 -0700
Subject: [PATCH] Add more tolerance for missing deps in
 maybeHandleRegisteringNewlyDiscoveredDepsForDoneEntry if the evaluator
 permits such an inconsistency.

PiperOrigin-RevId: 263125555
---
 .../skyframe/AbstractParallelEvaluator.java   | 70 ++++++++++---------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java
index 9012f14e9d788b..879f51dc76fe8d 100644
--- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java
@@ -940,29 +940,22 @@ private boolean maybeHandleRegisteringNewlyDiscoveredDepsForDoneEntry(
     InterruptibleSupplier<Map<SkyKey, ? extends NodeEntry>> newlyAddedNewDepNodes =
         graph.getBatchAsync(skyKey, Reason.RDEP_ADDITION, newlyAddedNewDeps);
 
-    // Note that the depEntries in the following two loops can't be null. In a keep-going build, we
-    // normally expect all deps to be done. In a non-keep-going build, If env.newlyRequestedDeps
-    // contained a key for a node that wasn't done, then it would have been removed via
-    // removeUndoneNewlyRequestedDeps() just above this loop. However, with intra-evaluation
-    // dirtying, a dep may not be done.
+    // Dep entries in the following two loops may not be done, but they must be present. If the
+    // graph permits an already declared child missing, we recreate the entry if necessary. In a
+    // keep-going build, we normally expect all deps to be done. In a non-keep-going build, if
+    // env.newlyRequestedDeps contained a key for a node that wasn't done, then it would have been
+    // removed via removeUndoneNewlyRequestedDeps() just above this loop. However, with
+    // intra-evaluation dirtying, a dep may not be done.
     boolean dirtyDepFound = false;
     boolean selfSignalled = false;
+
     Map<SkyKey, ? extends NodeEntry> previouslyRegisteredEntries =
         graph.getBatch(skyKey, Reason.SIGNAL_DEP, previouslyRegisteredNewDeps);
-    if (previouslyRegisteredEntries.size() != previouslyRegisteredNewDeps.size()) {
-      throw new IllegalStateException(
-          "Missing entries that were already known about: "
-              + Sets.difference(previouslyRegisteredNewDeps, previouslyRegisteredEntries.keySet())
-              + " for "
-              + skyKey
-              + " with entry "
-              + entry);
-    }
-    for (Map.Entry<SkyKey, ? extends NodeEntry> newDep : previouslyRegisteredEntries.entrySet()) {
-      NodeEntry depEntry = newDep.getValue();
+    for (SkyKey newDep : previouslyRegisteredNewDeps) {
+      NodeEntry depEntry =
+          getOrRecreateDepEntry(newDep, previouslyRegisteredEntries, skyKey, Reason.SIGNAL_DEP);
       DependencyState triState = depEntry.checkIfDoneForDirtyReverseDep(skyKey);
-      switch (maybeHandleUndoneDepForDoneEntry(
-          entry, depEntry, triState, skyKey, newDep.getKey())) {
+      switch (maybeHandleUndoneDepForDoneEntry(entry, depEntry, triState, skyKey, newDep)) {
         case DEP_DONE_SELF_SIGNALLED:
           selfSignalled = true;
           break;
@@ -975,21 +968,8 @@ private boolean maybeHandleRegisteringNewlyDiscoveredDepsForDoneEntry(
     }
 
     for (SkyKey newDep : newlyAddedNewDeps) {
-      NodeEntry depEntry = newlyAddedNewDepNodes.get().get(newDep);
-      // newlyAddedNewDepNodes should generally have all of the nodes requested. If it does not, and
-      // the evaluator permits such an inconsistency, fall back to creating the node.
-      if (depEntry == null) {
-        List<SkyKey> missing = ImmutableList.of(newDep);
-        evaluatorContext
-            .getGraphInconsistencyReceiver()
-            .noteInconsistencyAndMaybeThrow(
-                skyKey, missing, Inconsistency.ALREADY_DECLARED_CHILD_MISSING);
-        depEntry =
-            Preconditions.checkNotNull(
-                graph.createIfAbsentBatch(skyKey, Reason.RDEP_ADDITION, missing).get(newDep),
-                newDep);
-      }
-
+      NodeEntry depEntry =
+          getOrRecreateDepEntry(newDep, newlyAddedNewDepNodes.get(), skyKey, Reason.RDEP_ADDITION);
       DependencyState triState = depEntry.addReverseDepAndCheckIfDone(skyKey);
       switch (maybeHandleUndoneDepForDoneEntry(entry, depEntry, triState, skyKey, newDep)) {
         case DEP_DONE_SELF_SIGNALLED:
@@ -1014,6 +994,30 @@ private boolean maybeHandleRegisteringNewlyDiscoveredDepsForDoneEntry(
     return !selfSignalled;
   }
 
+  /**
+   * Returns a {@link NodeEntry} for {@code depKey}.
+   *
+   * <p>If {@code depKey} is present in {@code depEntries}, its corresponding entry is returned.
+   * Otherwise, if the evaluator permits {@link Inconsistency#ALREADY_DECLARED_CHILD_MISSING}, the
+   * entry will be recreated.
+   */
+  private NodeEntry getOrRecreateDepEntry(
+      SkyKey depKey, Map<SkyKey, ? extends NodeEntry> depEntries, SkyKey requestor, Reason reason)
+      throws InterruptedException {
+    NodeEntry depEntry = depEntries.get(depKey);
+    if (depEntry == null) {
+      List<SkyKey> missing = ImmutableList.of(depKey);
+      evaluatorContext
+          .getGraphInconsistencyReceiver()
+          .noteInconsistencyAndMaybeThrow(
+              depKey, missing, Inconsistency.ALREADY_DECLARED_CHILD_MISSING);
+      depEntry =
+          Preconditions.checkNotNull(
+              graph.createIfAbsentBatch(requestor, reason, missing).get(depKey), depKey);
+    }
+    return depEntry;
+  }
+
   private enum MaybeHandleUndoneDepResult {
     DEP_DONE_SELF_SIGNALLED,
     DEP_DONE_SELF_NOT_SIGNALLED,