Make NodeEntry#addExternalDep abstract instead of throwing by default.
This is more consistent with other methods in NodeEntry and forces implementations to declare whether it is actually unsupported, reducing the likelihood of unintentionally inheriting a throwing implementation.
Test case is moved from ParallelEvaluatorTest to MemoizingEvaluatorTest for better coverage of internal code.
RELNOTES: None.
PiperOrigin-RevId: 235019061
diff --git a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
index 5b60c36..7b0e26b 100644
--- a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
@@ -428,9 +428,7 @@
@ThreadSafe
void addTemporaryDirectDepsGroupToDirtyEntry(List<SkyKey> group);
- default void addExternalDep() {
- throw new UnsupportedOperationException();
- }
+ void addExternalDep();
/**
* Returns true if the node is ready to be evaluated, i.e., it has been signaled exactly as many
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
index a011d22..67605ec 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -36,6 +36,7 @@
import com.google.common.eventbus.EventBus;
import com.google.common.testing.GcFinalization;
import com.google.common.truth.IterableSubject;
+import com.google.common.util.concurrent.SettableFuture;
import com.google.common.util.concurrent.Uninterruptibles;
import com.google.devtools.build.lib.events.DelegatingEventHandler;
import com.google.devtools.build.lib.events.Event;
@@ -5039,6 +5040,118 @@
runTestDuplicateUnfinishedDeps(/*keepGoing=*/ true);
}
+ @Test
+ public void externalDep() throws Exception {
+ externalDep(1, 0);
+ externalDep(2, 0);
+ externalDep(1, 1);
+ externalDep(1, 2);
+ externalDep(2, 1);
+ externalDep(2, 2);
+ }
+
+ private void externalDep(int firstPassCount, int secondPassCount) throws Exception {
+ final SkyKey parentKey = toSkyKey("parentKey");
+ final CountDownLatch firstPassLatch = new CountDownLatch(1);
+ final CountDownLatch secondPassLatch = new CountDownLatch(1);
+ tester
+ .getOrCreate(parentKey)
+ .setBuilder(
+ new SkyFunction() {
+ // Skyframe doesn't have native support for continuations, so we use fields here. A
+ // simple continuation API in Skyframe could be Environment providing a
+ // setContinuation(SkyContinuation) method, where SkyContinuation provides a compute
+ // method similar to SkyFunction. When restarting the node, Skyframe would then call
+ // the continuation rather than the original SkyFunction. If we do that, we should
+ // consider only allowing calls to dependOnFuture in combination with setContinuation.
+ private List<SettableFuture<SkyValue>> firstPass;
+ private List<SettableFuture<SkyValue>> secondPass;
+
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env) {
+ if (firstPass == null) {
+ firstPass = new ArrayList<>();
+ for (int i = 0; i < firstPassCount; i++) {
+ SettableFuture<SkyValue> future = SettableFuture.create();
+ firstPass.add(future);
+ env.dependOnFuture(future);
+ }
+ assertThat(env.valuesMissing()).isTrue();
+ Thread helper =
+ new Thread(
+ () -> {
+ try {
+ firstPassLatch.await();
+ for (int i = 0; i < firstPassCount; i++) {
+ firstPass.get(i).set(new StringValue("value1"));
+ }
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ });
+ helper.start();
+ return null;
+ } else if (secondPass == null && secondPassCount > 0) {
+ for (int i = 0; i < firstPassCount; i++) {
+ assertThat(firstPass.get(i).isDone()).isTrue();
+ }
+ secondPass = new ArrayList<>();
+ for (int i = 0; i < secondPassCount; i++) {
+ SettableFuture<SkyValue> future = SettableFuture.create();
+ secondPass.add(future);
+ env.dependOnFuture(future);
+ }
+ assertThat(env.valuesMissing()).isTrue();
+ Thread helper =
+ new Thread(
+ () -> {
+ try {
+ secondPassLatch.await();
+ for (int i = 0; i < secondPassCount; i++) {
+ secondPass.get(i).set(new StringValue("value2"));
+ }
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ });
+ helper.start();
+ return null;
+ }
+ for (int i = 0; i < secondPassCount; i++) {
+ assertThat(secondPass.get(i).isDone()).isTrue();
+ }
+ return new StringValue("done!");
+ }
+
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ });
+ tester.evaluator.injectGraphTransformerForTesting(
+ NotifyingHelper.makeNotifyingTransformer(
+ new Listener() {
+ private boolean firstPassDone;
+
+ @Override
+ public void accept(SkyKey key, EventType type, Order order, Object context) {
+ // NodeEntry.addExternalDep is called as part of bookkeeping at the end of
+ // AbstractParallelEvaluator.Evaluate#run.
+ if (key == parentKey && type == EventType.ADD_EXTERNAL_DEP) {
+ if (!firstPassDone) {
+ firstPassLatch.countDown();
+ firstPassDone = true;
+ } else {
+ secondPassLatch.countDown();
+ }
+ }
+ }
+ }));
+ EvaluationResult<StringValue> result = tester.eval(/*keepGoing=*/ false, parentKey);
+ assertThat(result.hasError()).isFalse();
+ assertThat(result.get(parentKey)).isEqualTo(new StringValue("done!"));
+ }
+
private void runTestDuplicateUnfinishedDeps(boolean keepGoing) throws Exception {
SkyKey parentKey = GraphTester.skyKey("parent");
SkyKey childKey = GraphTester.skyKey("child");
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 0a19689..cb4dcae 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -164,120 +164,6 @@
}
@Test
- public void externalDep() throws Exception {
- externalDep(1, 0);
- externalDep(2, 0);
- externalDep(1, 1);
- externalDep(1, 2);
- externalDep(2, 1);
- externalDep(2, 2);
- }
-
- private void externalDep(int firstPassCount, int secondPassCount) throws Exception {
- final SkyKey parentKey = GraphTester.toSkyKey("parentKey");
- final CountDownLatch firstPassLatch = new CountDownLatch(1);
- final CountDownLatch secondPassLatch = new CountDownLatch(1);
- tester
- .getOrCreate(parentKey)
- .setBuilder(
- new SkyFunction() {
- // Skyframe doesn't have native support for continuations, so we use fields here. A
- // simple continuation API in Skyframe could be Environment providing a
- // setContinuation(SkyContinuation) method, where SkyContinuation provides a compute
- // method similar to SkyFunction. When restarting the node, Skyframe would then call
- // the continuation rather than the original SkyFunction. If we do that, we should
- // consider only allowing calls to dependOnFuture in combination with setContinuation.
- private List<SettableFuture<SkyValue>> firstPass;
- private List<SettableFuture<SkyValue>> secondPass;
-
- @Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
- if (firstPass == null) {
- firstPass = new ArrayList<>();
- for (int i = 0; i < firstPassCount; i++) {
- SettableFuture<SkyValue> future = SettableFuture.create();
- firstPass.add(future);
- env.dependOnFuture(future);
- }
- assertThat(env.valuesMissing()).isTrue();
- Thread helper =
- new Thread(
- () -> {
- try {
- firstPassLatch.await();
- // Thread.sleep(5);
- for (int i = 0; i < firstPassCount; i++) {
- firstPass.get(i).set(new StringValue("value1"));
- }
- } catch (InterruptedException e) {
- throw new RuntimeException(e);
- }
- });
- helper.start();
- return null;
- } else if (secondPass == null && secondPassCount > 0) {
- for (int i = 0; i < firstPassCount; i++) {
- assertThat(firstPass.get(i).isDone()).isTrue();
- }
- secondPass = new ArrayList<>();
- for (int i = 0; i < secondPassCount; i++) {
- SettableFuture<SkyValue> future = SettableFuture.create();
- secondPass.add(future);
- env.dependOnFuture(future);
- }
- assertThat(env.valuesMissing()).isTrue();
- Thread helper =
- new Thread(
- () -> {
- try {
- secondPassLatch.await();
- for (int i = 0; i < secondPassCount; i++) {
- secondPass.get(i).set(new StringValue("value2"));
- }
- } catch (InterruptedException e) {
- throw new RuntimeException(e);
- }
- });
- helper.start();
- return null;
- }
- for (int i = 0; i < secondPassCount; i++) {
- assertThat(secondPass.get(i).isDone()).isTrue();
- }
- return new StringValue("done!");
- }
-
- @Override
- public String extractTag(SkyKey skyKey) {
- return null;
- }
- });
- graph =
- NotifyingHelper.makeNotifyingTransformer(
- new Listener() {
- private boolean firstPassDone;
-
- @Override
- public void accept(SkyKey key, EventType type, Order order, Object context) {
- // NodeEntry.addExternalDep is called as part of bookkeeping at the end of
- // AbstractParallelEvaluator.Evaluate#run.
- if (key == parentKey && type == EventType.ADD_EXTERNAL_DEP) {
- if (!firstPassDone) {
- firstPassLatch.countDown();
- firstPassDone = true;
- } else {
- secondPassLatch.countDown();
- }
- }
- }
- })
- .transform(new InMemoryGraphImpl());
- EvaluationResult<StringValue> result = eval(/*keepGoing=*/ false, ImmutableList.of(parentKey));
- assertThat(result.hasError()).isFalse();
- assertThat(result.get(parentKey)).isEqualTo(new StringValue("done!"));
- }
-
- @Test
public void enqueueDoneFuture() throws Exception {
final SkyKey parentKey = GraphTester.toSkyKey("parentKey");
tester
