From 889f5fd7ab5c1461ce823ee9a3e315627c8b3698 Mon Sep 17 00:00:00 2001 From: janakr Date: Thu, 11 Apr 2019 12:28:07 -0700 Subject: [PATCH] Attempt to tolerate missing keys a bit more gracefully: reset the current node to restart from scratch, rather than continuing on an almost certainly doomed path, where we'll crash later. This is explicitly abusing the action rewinding framework to mitigate a different bug. PiperOrigin-RevId: 243119106 --- .../skyframe/AbstractParallelEvaluator.java | 4 +-- .../build/skyframe/SimpleCycleDetector.java | 6 ++--- .../skyframe/SkyFunctionEnvironment.java | 27 ++++++++++--------- 3 files changed, 19 insertions(+), 18 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 79c0cdb9c00210..cc0e0386d6f36e 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java @@ -40,7 +40,7 @@ import com.google.devtools.build.skyframe.ParallelEvaluatorContext.EnqueueParentBehavior; import com.google.devtools.build.skyframe.QueryableGraph.Reason; import com.google.devtools.build.skyframe.SkyFunction.Restart; -import com.google.devtools.build.skyframe.SkyFunctionEnvironment.UndonePreviouslyRequestedDep; +import com.google.devtools.build.skyframe.SkyFunctionEnvironment.UndonePreviouslyRequestedDeps; import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException; import com.google.devtools.build.skyframe.ThinNodeEntry.DirtyType; import java.math.BigInteger; @@ -425,7 +425,7 @@ public void run() { env = new SkyFunctionEnvironment( skyKey, state.getTemporaryDirectDeps(), oldDeps, evaluatorContext); - } catch (UndonePreviouslyRequestedDep undonePreviouslyRequestedDep) { + } catch (UndonePreviouslyRequestedDeps undonePreviouslyRequestedDeps) { // If a previously requested dep is no longer done, restart this node from scratch. restart(skyKey, state); // Top priority since this node has already been evaluating, so get it off our plate. diff --git a/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java b/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java index fecf35920adcc8..732fc4c94cb6d1 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java +++ b/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java @@ -26,7 +26,7 @@ import com.google.devtools.build.lib.util.GroupedList; import com.google.devtools.build.skyframe.ParallelEvaluatorContext.EnqueueParentBehavior; import com.google.devtools.build.skyframe.QueryableGraph.Reason; -import com.google.devtools.build.skyframe.SkyFunctionEnvironment.UndonePreviouslyRequestedDep; +import com.google.devtools.build.skyframe.SkyFunctionEnvironment.UndonePreviouslyRequestedDeps; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Deque; @@ -156,12 +156,12 @@ private static ErrorInfo checkForCycles(SkyKey root, ParallelEvaluatorContext ev directDeps, Sets.difference(entry.getAllRemainingDirtyDirectDeps(), removedDeps), evaluatorContext); - } catch (UndonePreviouslyRequestedDep undoneDep) { + } catch (UndonePreviouslyRequestedDeps undoneDep) { // All children were finished according to the CHILDREN_FINISHED sentinel, and cycle // detection does not do normal SkyFunction evaluation, so no restarting nor child // dirtying was possible. throw new IllegalStateException( - "Previously requested dep not done: " + undoneDep.getDepKey(), undoneDep); + "Previously requested dep not done: " + undoneDep.getDepKeys(), undoneDep); } env.setError(entry, ErrorInfo.fromChildErrors(key, errorDeps)); env.commit(entry, EnqueueParentBehavior.SIGNAL); diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java index e0514d2b80d98d..5a80ac4a52106b 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java @@ -162,7 +162,7 @@ public void post(ExtendedEventHandler.Postable e) { GroupedList directDeps, Set oldDeps, ParallelEvaluatorContext evaluatorContext) - throws InterruptedException, UndonePreviouslyRequestedDep { + throws InterruptedException, UndonePreviouslyRequestedDeps { super(directDeps); this.skyKey = skyKey; this.oldDeps = oldDeps; @@ -193,10 +193,10 @@ public void post(ExtendedEventHandler.Postable e) { try { this.previouslyRequestedDepsValues = batchPrefetch(skyKey, directDeps, oldDeps, /*assertDone=*/ false); - } catch (UndonePreviouslyRequestedDep undonePreviouslyRequestedDep) { + } catch (UndonePreviouslyRequestedDeps undonePreviouslyRequestedDeps) { throw new IllegalStateException( - "batchPrefetch can't throw UndonePreviouslyRequestedDep unless assertDone is true", - undonePreviouslyRequestedDep); + "batchPrefetch can't throw UndonePreviouslyRequestedDeps unless assertDone is true", + undonePreviouslyRequestedDeps); } Preconditions.checkState( !this.previouslyRequestedDepsValues.containsKey(ErrorTransienceValue.KEY), @@ -206,7 +206,7 @@ public void post(ExtendedEventHandler.Postable e) { private Map batchPrefetch( SkyKey requestor, GroupedList depKeys, Set oldDeps, boolean assertDone) - throws InterruptedException, UndonePreviouslyRequestedDep { + throws InterruptedException, UndonePreviouslyRequestedDeps { QueryableGraph.PrefetchDepsRequest request = null; if (PREFETCH_OLD_DEPS) { request = new QueryableGraph.PrefetchDepsRequest(requestor, oldDeps, depKeys); @@ -235,6 +235,7 @@ private Map batchPrefetch( .getGraphInconsistencyReceiver() .noteInconsistencyAndMaybeThrow( requestor, difference, Inconsistency.ALREADY_DECLARED_CHILD_MISSING); + throw new UndonePreviouslyRequestedDeps(ImmutableList.copyOf(difference)); } ImmutableMap.Builder depValuesBuilder = ImmutableMap.builderWithExpectedSize(batchMap.size()); @@ -251,7 +252,7 @@ private Map batchPrefetch( skyKey, ImmutableList.of(entry.getKey()), Inconsistency.BUILDING_PARENT_FOUND_UNDONE_CHILD); - throw new UndonePreviouslyRequestedDep(entry.getKey()); + throw new UndonePreviouslyRequestedDeps(ImmutableList.of(entry.getKey())); } depValuesBuilder.put(entry.getKey(), !depDone ? NULL_MARKER : valueMaybeWithMetadata); if (depDone) { @@ -902,16 +903,16 @@ public String toString() { .toString(); } - /** Thrown during environment construction if a previously requested dep is no longer done. */ - static class UndonePreviouslyRequestedDep extends Exception { - private final SkyKey depKey; + /** Thrown during environment construction if previously requested deps are no longer done. */ + static class UndonePreviouslyRequestedDeps extends Exception { + private final ImmutableList depKeys; - UndonePreviouslyRequestedDep(SkyKey depKey) { - this.depKey = depKey; + UndonePreviouslyRequestedDeps(ImmutableList depKeys) { + this.depKeys = depKeys; } - SkyKey getDepKey() { - return depKey; + ImmutableList getDepKeys() { + return depKeys; } } }