Ensure PartialReevaluation cycle node's deps are fetched if necessary
Previously, in case when the cycle node supports partial reevaluation, all of cycle nodes deps are not fetched if `env.ensurePreviouslyRequestedDepsFetched()` is not called, which will cause further execution to fail. This is a very uncommon case, so it was never caught.
On the other hand, if cycle node does not support partial reevaluation, `env.ensurePreviouslyRequestedDepsFetched()` is just a no-op, meaning this change does not introduce side effect.
PiperOrigin-RevId: 625423771
Change-Id: Iec275e7c6a45ade18f7fd0daab44b15e8d3bbe16
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 d0fca7e..47d4a55 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SimpleCycleDetector.java
@@ -166,6 +166,10 @@
Sets.difference(entry.getAllRemainingDirtyDirectDeps(), removedDeps),
entry.getMaxTransitiveSourceVersion(),
evaluatorContext);
+ // When the environment sets a cycle node to be in error and commits afterwards, it
+ // requires all of its deps to be fetched. See `SkyFunctionEnvironment#setError()`'s
+ // JavaDoc for more details.
+ env.ensurePreviouslyRequestedDepsFetched();
} catch (UndonePreviouslyRequestedDeps undoneDeps) {
// All children were finished according to the CHILDREN_FINISHED sentinel, and cycle
// detection does not do normal SkyFunction evaluation, so no restarting nor child
diff --git a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
index 02efc54..dbfe6a4 100644
--- a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
+++ b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
@@ -419,6 +419,10 @@
return ImmutableMap.copyOf(functionMap);
}
+ public void putDelegateFunction(SkyFunctionName functionName) {
+ putSkyFunction(functionName, new DelegatingFunction());
+ }
+
public void putSkyFunction(SkyFunctionName functionName, SkyFunction function) {
functionMap.put(functionName, function);
}
diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
index 39f1dd9..a6579e2 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -1839,11 +1839,20 @@
assertThat(numComputes.get()).isEqualTo(2);
}
+ private SkyKey createCycleKey(String keyName, boolean isCycleNodePartialReevaluation) {
+ if (isCycleNodePartialReevaluation) {
+ tester.putDelegateFunction(PartialReevaluationKey.FUNCTION_NAME);
+ return new PartialReevaluationKey(keyName);
+ }
+ return skyKey(keyName);
+ }
+
/** Make sure that multiple unfinished children can be cleared from a cycle value. */
@Test
- public void cycleWithMultipleUnfinishedChildren() throws Exception {
+ public void cycleWithMultipleUnfinishedChildren(
+ @TestParameter boolean isCycleNodePartialReevaluation) throws Exception {
graph = new DeterministicHelper.DeterministicProcessableGraph(new InMemoryGraphImpl());
- SkyKey cycleKey = skyKey("zcycle");
+ SkyKey cycleKey = createCycleKey("cycle", isCycleNodePartialReevaluation);
SkyKey midKey = skyKey("mid");
SkyKey topKey = skyKey("top");
SkyKey selfEdge1 = skyKey("selfEdge1");
@@ -1873,11 +1882,13 @@
* and cycle. Error bubbles up from mid to cycle, and we should detect cycle.
*/
@Test
- public void cycleAndErrorInBubbleUp(@TestParameter boolean keepGoing) throws Exception {
+ public void cycleAndErrorInBubbleUp(
+ @TestParameter boolean keepGoing, @TestParameter boolean isCycleNodePartialReevaluation)
+ throws Exception {
graph = new DeterministicHelper.DeterministicProcessableGraph(new InMemoryGraphImpl());
tester = new GraphTester();
SkyKey errorKey = skyKey("error");
- SkyKey cycleKey = skyKey("cycle");
+ SkyKey cycleKey = createCycleKey("cycle", isCycleNodePartialReevaluation);
SkyKey midKey = skyKey("mid");
SkyKey topKey = skyKey("top");
tester.getOrCreate(topKey).addDependency(midKey).setComputedValue(CONCATENATE);
@@ -1982,11 +1993,13 @@
* error, just to mix it up.
*/
@Test
- public void cycleAndErrorAndError(@TestParameter boolean keepGoing) throws Exception {
+ public void cycleAndErrorAndError(
+ @TestParameter boolean keepGoing, @TestParameter boolean isCycleNodePartialReevaluation)
+ throws Exception {
graph = new DeterministicHelper.DeterministicProcessableGraph(new InMemoryGraphImpl());
tester = new GraphTester();
SkyKey errorKey = skyKey("error");
- SkyKey cycleKey = skyKey("cycle");
+ SkyKey cycleKey = createCycleKey("cycle", isCycleNodePartialReevaluation);
SkyKey midKey = skyKey("mid");
SkyKey topKey = skyKey("top");
tester.getOrCreate(topKey).addDependency(midKey).setComputedValue(CONCATENATE);
