Skip to content

Commit

Permalink
Merge pull request #23330 Only require ordinal nodes that are actuall…
Browse files Browse the repository at this point in the history
…y depended on

We have been seeing some issues with the task execution plan not being able to make progress. Looking at [the reproducer](https://github.com/gradle/gradle/blob/e5cc0915b12e88bfaf97b4065352cd48642af51c/subprojects/core/src/integTest/groovy/org/gradle/api/DestroyerTaskCommandLineOrderIntegrationTest.groovy#L451) from @wolfs, it appears the following is happening:

- we are adding producers to ordinal group 1
- this causes the destroyer ordinals from the previous group (ordinal group 0) [to be scheduled](https://github.com/gradle/gradle/blob/901ba375e5bab6cceb48820a69a1858f8128b5b7/subprojects/core/src/main/java/org/gradle/execution/plan/OrdinalNodeAccess.java#L59) but not the producer ordinal from group 1 (or any of its dependencies)
- when we update the dependencies complete for the last passing producer node from ordinal group 1, we [mark his dependents as ready](https://github.com/gradle/gradle/blob/901ba375e5bab6cceb48820a69a1858f8128b5b7/subprojects/core/src/main/java/org/gradle/execution/plan/DefaultFinalizedExecutionPlan.java#L364) which includes the producer node from group 1.
- the producer node from group 1 is added to the ready group (even though it wasn't originally scheduled), completes, but then, because we have --continue, it adds its dependencies (including the producer node from ordinal group 0) to the waiting list.
- Now we are waiting on the producer node from group 0, but it is not scheduled, so no progress.

This change addresses the problem by only changing the state of ordinal nodes to required (i.e. SHOULD_RUN) when a task node (or another ordinal node) from a later group adds it as a dependency.  This means that it won't get added to the ready queue later (since it is not required) and therefore it won't trigger the condition where we end up waiting on one of its dependencies.

This effectively reverts the changes from #23292.

Co-authored-by: Gary Hale <gary@gradle.com>
  • Loading branch information
bot-gradle and ghale committed Dec 29, 2022
2 parents 27686d5 + 0687bb8 commit 9fbe3a8
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -515,13 +515,6 @@ private void recordNodeCompleted(Node node) {
// Wait for any dependencies of this node that have not started yet
for (Node successor : node.getDependencySuccessors()) {
if (successor.isRequired()) {
// There may be a dependency of this node which does not have
// - any dependencies of its own,
// - and is not part of the initially scheduled nodes.
// We need to check if this node is ready to start, if not it will never start.
// An example of this is a producer node of an ordinal group, when there aren't any producers in the ordinal group.
successor.updateAllDependenciesComplete();
maybeNodeReady(successor);
waitingForNode(successor, "other node completed", node);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ public OrdinalNode getProducerLocationsNode() {
if (previous != null) {
producerLocationsNode.addDependencySuccessor(previous.getProducerLocationsNode());
}
producerLocationsNode.require();
}
return producerLocationsNode;
}
Expand All @@ -79,7 +78,6 @@ public OrdinalNode getDestroyerLocationsNode() {
if (previous != null) {
destroyerLocationsNode.addDependencySuccessor(previous.getDestroyerLocationsNode());
}
destroyerLocationsNode.require();
}
return destroyerLocationsNode;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ void addProducerNode(OrdinalGroup ordinal, LocalTaskNode producer, Consumer<Node

private void maybeSchedule(Consumer<Node> ordinalNodeConsumer, OrdinalNode node) {
if (requiredNodes.add(node)) {
node.require();
for (Node successor : node.getDependencySuccessors()) {
if (successor instanceof OrdinalNode) {
maybeSchedule(ordinalNodeConsumer, (OrdinalNode) successor);
Expand Down

0 comments on commit 9fbe3a8

Please sign in to comment.