Use TrackingAwaiter properly to track lost exceptions. Using the static method wasn't guaranteed to catch all bugs. Also convert to a singleton since there's no reason to have multiple instances.
--
MOS_MIGRATED_REVID=102158719
diff --git a/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java b/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java
index 4f87bb3..48325b0 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java
@@ -54,12 +54,12 @@
notifyStart.countDown();
}
if (waitToFinish != null) {
- TrackingAwaiter.waitAndMaybeThrowInterrupt(waitToFinish,
- key + " timed out waiting to finish");
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ waitToFinish, key + " timed out waiting to finish");
if (waitForException) {
SkyFunctionEnvironment skyEnv = (SkyFunctionEnvironment) env;
- TrackingAwaiter.waitAndMaybeThrowInterrupt(skyEnv.getExceptionLatchForTesting(),
- key + " timed out waiting for exception");
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ skyEnv.getExceptionLatchForTesting(), key + " timed out waiting for exception");
}
}
for (SkyKey dep : deps) {
diff --git a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
index c0d349d..aa9b036 100644
--- a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
@@ -40,6 +40,7 @@
import com.google.devtools.build.skyframe.InvalidatingNodeVisitor.InvalidationState;
import com.google.devtools.build.skyframe.InvalidatingNodeVisitor.InvalidationType;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.experimental.runners.Enclosed;
@@ -69,6 +70,11 @@
private IntVersion graphVersion = new IntVersion(0);
+ @After
+ public void assertNoTrackedErrors() {
+ TrackingAwaiter.INSTANCE.assertNoErrors();
+ }
+
// The following three methods should be abstract, but junit4 does not allow us to run inner
// classes in an abstract outer class. Thus, we provide implementations. These methods will never
// be run because only the inner classes, annotated with @RunWith, will actually be executed.
@@ -466,37 +472,34 @@
}
int countDownStart = validValuesToDo > 0 ? random.nextInt(validValuesToDo) : 0;
final CountDownLatch countDownToInterrupt = new CountDownLatch(countDownStart);
- final EvaluationProgressReceiver receiver = new EvaluationProgressReceiver() {
- @Override
- public void invalidated(SkyKey skyKey, InvalidationState state) {
- countDownToInterrupt.countDown();
- if (countDownToInterrupt.getCount() == 0) {
- mainThread.interrupt();
- try {
- // Wait for the main thread to be interrupted uninterruptibly, because the main thread
- // is going to interrupt us, and we don't want to get into an interrupt fight. Only
- // if we get interrupted without the main thread also being interrupted will this
- // throw an InterruptedException.
- TrackingAwaiter.waitAndMaybeThrowInterrupt(
- visitor.get().getInterruptionLatchForTestingOnly(),
- "Main thread was not interrupted");
- } catch (InterruptedException e) {
- throw new IllegalStateException(e);
+ final EvaluationProgressReceiver receiver =
+ new EvaluationProgressReceiver() {
+ @Override
+ public void invalidated(SkyKey skyKey, InvalidationState state) {
+ countDownToInterrupt.countDown();
+ if (countDownToInterrupt.getCount() == 0) {
+ mainThread.interrupt();
+ // Wait for the main thread to be interrupted uninterruptibly, because the main
+ // thread is going to interrupt us, and we don't want to get into an interrupt
+ // fight. Only if we get interrupted without the main thread also being interrupted
+ // will this throw an InterruptedException.
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ visitor.get().getInterruptionLatchForTestingOnly(),
+ "Main thread was not interrupted");
+ }
}
- }
- }
- @Override
- public void enqueueing(SkyKey skyKey) {
- throw new UnsupportedOperationException();
- }
+ @Override
+ public void enqueueing(SkyKey skyKey) {
+ throw new UnsupportedOperationException();
+ }
- @Override
- public void evaluated(SkyKey skyKey, Supplier<SkyValue> skyValueSupplier,
- EvaluationState state) {
- throw new UnsupportedOperationException();
- }
- };
+ @Override
+ public void evaluated(
+ SkyKey skyKey, Supplier<SkyValue> skyValueSupplier, EvaluationState state) {
+ throw new UnsupportedOperationException();
+ }
+ };
try {
invalidate(graph, receiver,
Sets.newHashSet(
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 9199204..c75af92 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -55,6 +55,7 @@
import com.google.devtools.build.skyframe.NotifyingInMemoryGraph.Order;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
+import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
@@ -96,6 +97,11 @@
initializeTester(null);
}
+ @After
+ public void assertNoTrackedErrors() {
+ TrackingAwaiter.INSTANCE.assertNoErrors();
+ }
+
public void initializeTester(@Nullable TrackingInvalidationReceiver customInvalidationReceiver) {
emittedEventState = new MemoizingEvaluator.EmittedEventState();
tester = new MemoizingEvaluatorTester();
@@ -544,34 +550,35 @@
public void alreadyAnalyzedBadTarget() throws Exception {
final SkyKey mid = GraphTester.toSkyKey("mid");
final CountDownLatch valueSet = new CountDownLatch(1);
- final TrackingAwaiter trackingAwaiter = new TrackingAwaiter();
- setGraphForTesting(new NotifyingInMemoryGraph(new Listener() {
- @Override
- public void accept(SkyKey key, EventType type, Order order, Object context) {
- if (!key.equals(mid)) {
- return;
- }
- switch (type) {
- case ADD_REVERSE_DEP:
- if (context == null) {
- // Context is null when we are enqueuing this value as a top-level job.
- trackingAwaiter.awaitLatchAndTrackExceptions(valueSet, "value not set");
- }
- break;
- case SET_VALUE:
- valueSet.countDown();
- break;
- default:
- break;
- }
- }
- }));
+ setGraphForTesting(
+ new NotifyingInMemoryGraph(
+ new Listener() {
+ @Override
+ public void accept(SkyKey key, EventType type, Order order, Object context) {
+ if (!key.equals(mid)) {
+ return;
+ }
+ switch (type) {
+ case ADD_REVERSE_DEP:
+ if (context == null) {
+ // Context is null when we are enqueuing this value as a top-level job.
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ valueSet, "value not set");
+ }
+ break;
+ case SET_VALUE:
+ valueSet.countDown();
+ break;
+ default:
+ break;
+ }
+ }
+ }));
SkyKey top = GraphTester.skyKey("top");
tester.getOrCreate(top).addDependency(mid).setComputedValue(CONCATENATE);
tester.getOrCreate(mid).setHasError(true);
tester.eval(/*keepGoing=*/false, top, mid);
assertEquals(0L, valueSet.getCount());
- trackingAwaiter.assertNoErrors();
assertThat(tester.invalidationReceiver.evaluated).containsExactly(mid);
}
@@ -892,22 +899,25 @@
final CountDownLatch errorThrown = new CountDownLatch(1);
// We don't do anything on the first build.
final AtomicBoolean secondBuild = new AtomicBoolean(false);
- final TrackingAwaiter trackingAwaiter = new TrackingAwaiter();
- setGraphForTesting(new DeterministicInMemoryGraph(new Listener() {
- @Override
- public void accept(SkyKey key, EventType type, Order order, Object context) {
- if (!secondBuild.get()) {
- return;
- }
- if (key.equals(otherTop) && type == EventType.SIGNAL) {
- // otherTop is being signaled that dep1 is done. Tell the error value that it is ready,
- // then wait until the error is thrown, so that otherTop's builder is not re-entered.
- valuesReady.countDown();
- trackingAwaiter.awaitLatchAndTrackExceptions(errorThrown, "error not thrown");
- return;
- }
- }
- }));
+ setGraphForTesting(
+ new DeterministicInMemoryGraph(
+ new Listener() {
+ @Override
+ public void accept(SkyKey key, EventType type, Order order, Object context) {
+ if (!secondBuild.get()) {
+ return;
+ }
+ if (key.equals(otherTop) && type == EventType.SIGNAL) {
+ // otherTop is being signaled that dep1 is done. Tell the error value that it is
+ // ready, then wait until the error is thrown, so that otherTop's builder is not
+ // re-entered.
+ valuesReady.countDown();
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ errorThrown, "error not thrown");
+ return;
+ }
+ }
+ }));
final SkyKey dep1 = GraphTester.toSkyKey("dep1");
tester.set(dep1, new StringValue("dep1"));
final SkyKey dep2 = GraphTester.toSkyKey("dep2");
@@ -957,7 +967,6 @@
// they appear here.
EvaluationResult<StringValue> result =
tester.eval(/*keepGoing=*/false, otherTop, topKey, exceptionMarker);
- trackingAwaiter.assertNoErrors();
assertThat(result.errorMap().keySet()).containsExactly(topKey);
Iterable<CycleInfo> cycleInfos = result.getError(topKey).getCycleInfo();
assertWithMessage(result.toString()).that(cycleInfos).isNotEmpty();
@@ -1453,29 +1462,32 @@
final AtomicBoolean delayTopSignaling = new AtomicBoolean(false);
final CountDownLatch topSignaled = new CountDownLatch(1);
final CountDownLatch topRestartedBuild = new CountDownLatch(1);
- final TrackingAwaiter trackingAwaiter = new TrackingAwaiter();
- setGraphForTesting(new DeterministicInMemoryGraph(new Listener() {
- @Override
- public void accept(SkyKey key, EventType type, Order order, Object context) {
- if (!delayTopSignaling.get()) {
- return;
- }
- if (key.equals(top) && type == EventType.SIGNAL && order == Order.AFTER) {
- // top is signaled by firstKey (since slowAddingDep is blocking), so slowAddingDep is now
- // free to acknowledge top as a parent.
- topSignaled.countDown();
- return;
- }
- if (key.equals(slowAddingDep) && type == EventType.ADD_REVERSE_DEP
- && context.equals(top) && order == Order.BEFORE) {
- // If top is trying to declare a dep on slowAddingDep, wait until firstKey has signaled
- // top. Then this add dep will return DONE and top will be signaled, making it ready, so
- // it will be enqueued.
- trackingAwaiter.awaitLatchAndTrackExceptions(topSignaled,
- "first key didn't signal top in time");
- }
- }
- }));
+ setGraphForTesting(
+ new DeterministicInMemoryGraph(
+ new Listener() {
+ @Override
+ public void accept(SkyKey key, EventType type, Order order, Object context) {
+ if (!delayTopSignaling.get()) {
+ return;
+ }
+ if (key.equals(top) && type == EventType.SIGNAL && order == Order.AFTER) {
+ // top is signaled by firstKey (since slowAddingDep is blocking), so slowAddingDep
+ // is now free to acknowledge top as a parent.
+ topSignaled.countDown();
+ return;
+ }
+ if (key.equals(slowAddingDep)
+ && type == EventType.ADD_REVERSE_DEP
+ && context.equals(top)
+ && order == Order.BEFORE) {
+ // If top is trying to declare a dep on slowAddingDep, wait until firstKey has
+ // signaled top. Then this add dep will return DONE and top will be signaled,
+ // making it ready, so it will be enqueued.
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ topSignaled, "first key didn't signal top in time");
+ }
+ }
+ }));
// Value that is modified on the second build. Its thread won't finish until it signals top,
// which will wait for the signal before it enqueues its next dep. We prevent the thread from
// finishing by having the listener to which it reports its warning block until top's builder
@@ -1498,18 +1510,19 @@
return env.valuesMissing() ? null : new StringValue("top");
}
});
- reporter = new DelegatingEventHandler(reporter) {
- @Override
- public void handle(Event e) {
- super.handle(e);
- if (e.getKind() == EventKind.WARNING) {
- if (!throwError) {
- trackingAwaiter.awaitLatchAndTrackExceptions(topRestartedBuild,
- "top's builder did not start in time");
+ reporter =
+ new DelegatingEventHandler(reporter) {
+ @Override
+ public void handle(Event e) {
+ super.handle(e);
+ if (e.getKind() == EventKind.WARNING) {
+ if (!throwError) {
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ topRestartedBuild, "top's builder did not start in time");
+ }
+ }
}
- }
- }
- };
+ };
// First build : just prime the graph.
EvaluationResult<StringValue> result = tester.eval(/*keepGoing=*/false, top);
assertFalse(result.hasError());
@@ -1522,7 +1535,6 @@
tester.invalidate();
delayTopSignaling.set(true);
result = tester.eval(/*keepGoing=*/false, top);
- trackingAwaiter.assertNoErrors();
if (throwError) {
assertTrue(result.hasError());
assertThat(result.keyNames()).isEmpty(); // No successfully evaluated values.
@@ -1663,38 +1675,39 @@
// changed thread checks value entry once (to see if it is changed). dirty thread checks twice,
// to see if it is changed, and if it is dirty.
final CountDownLatch threadsStarted = new CountDownLatch(3);
- final TrackingAwaiter trackingAwaiter = new TrackingAwaiter();
- setGraphForTesting(new NotifyingInMemoryGraph(new Listener() {
- @Override
- public void accept(SkyKey key, EventType type, Order order, Object context) {
- if (!blockingEnabled.get()) {
- return;
- }
- if (!key.equals(parent)) {
- return;
- }
- if (type == EventType.IS_CHANGED && order == Order.BEFORE) {
- threadsStarted.countDown();
- }
- // Dirtiness only checked by dirty thread.
- if (type == EventType.IS_DIRTY && order == Order.BEFORE) {
- threadsStarted.countDown();
- }
- if (type == EventType.MARK_DIRTY) {
- trackingAwaiter.awaitLatchAndTrackExceptions(threadsStarted,
- "Both threads did not query if value isChanged in time");
- boolean isChanged = (Boolean) context;
- if (order == Order.BEFORE && !isChanged) {
- trackingAwaiter.awaitLatchAndTrackExceptions(waitForChanged,
- "'changed' thread did not mark value changed in time");
- return;
- }
- if (order == Order.AFTER && isChanged) {
- waitForChanged.countDown();
- }
- }
- }
- }));
+ setGraphForTesting(
+ new NotifyingInMemoryGraph(
+ new Listener() {
+ @Override
+ public void accept(SkyKey key, EventType type, Order order, Object context) {
+ if (!blockingEnabled.get()) {
+ return;
+ }
+ if (!key.equals(parent)) {
+ return;
+ }
+ if (type == EventType.IS_CHANGED && order == Order.BEFORE) {
+ threadsStarted.countDown();
+ }
+ // Dirtiness only checked by dirty thread.
+ if (type == EventType.IS_DIRTY && order == Order.BEFORE) {
+ threadsStarted.countDown();
+ }
+ if (type == EventType.MARK_DIRTY) {
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ threadsStarted, "Both threads did not query if value isChanged in time");
+ boolean isChanged = (Boolean) context;
+ if (order == Order.BEFORE && !isChanged) {
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ waitForChanged, "'changed' thread did not mark value changed in time");
+ return;
+ }
+ if (order == Order.AFTER && isChanged) {
+ waitForChanged.countDown();
+ }
+ }
+ }
+ }));
SkyKey leaf = GraphTester.toSkyKey("leaf");
tester.set(leaf, new StringValue("leaf"));
tester.getOrCreate(parent).addDependency(leaf).setComputedValue(CONCATENATE);
@@ -1712,7 +1725,6 @@
blockingEnabled.set(true);
result = tester.eval(/*keepGoing=*/false, parent);
assertEquals("leafother2", result.get(parent).getValue());
- trackingAwaiter.assertNoErrors();
assertEquals(0, waitForChanged.getCount());
assertEquals(0, threadsStarted.getCount());
}
@@ -2977,7 +2989,6 @@
// Keep track of any exceptions thrown during evaluation.
final AtomicReference<Pair<SkyKey, ? extends Exception>> unexpectedException =
new AtomicReference<>();
- final TrackingAwaiter trackingAwaiter = new TrackingAwaiter();
setGraphForTesting(
new DeterministicInMemoryGraph(
new Listener() {
@@ -2990,7 +3001,7 @@
|| type != EventType.SIGNAL) {
return;
}
- trackingAwaiter.awaitLatchAndTrackExceptions(
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
shutdownAwaiterStarted, "shutdown awaiter not started");
if (key.equals(uncachedParentKey)) {
// When the uncached parent is first signaled by its changed dep, make sure that
@@ -3062,15 +3073,10 @@
new SkyFunction() {
@Override
public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
- try {
- shutdownAwaiterStarted.countDown();
- TrackingAwaiter.waitAndMaybeThrowInterrupt(
- ((ParallelEvaluator.SkyFunctionEnvironment) env)
- .getExceptionLatchForTesting(),
- "");
- } catch (InterruptedException e) {
- unexpectedException.set(Pair.of(skyKey, e));
- }
+ shutdownAwaiterStarted.countDown();
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ ((ParallelEvaluator.SkyFunctionEnvironment) env).getExceptionLatchForTesting(),
+ "exception not thrown");
// Threadpool is shutting down. Don't try to synchronize anything in the future
// during error bubbling.
synchronizeThreads.set(false);
@@ -3089,7 +3095,6 @@
throw new AssertionError(unexpected.first + ", " + unexpected.second + ", "
+ Arrays.toString(unexpected.second.getStackTrace()));
}
- trackingAwaiter.assertNoErrors();
}
@Test
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 5d90a50..4ca6550 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -53,6 +53,7 @@
import com.google.devtools.build.skyframe.NotifyingInMemoryGraph.Order;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
+import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
@@ -93,6 +94,11 @@
reporter = new Reporter(eventCollector);
}
+ @After
+ public void assertNoTrackedErrors() {
+ TrackingAwaiter.INSTANCE.assertNoErrors();
+ }
+
private ParallelEvaluator makeEvaluator(ProcessableGraph graph,
ImmutableMap<SkyFunctionName, ? extends SkyFunction> builders, boolean keepGoing,
Predicate<Event> storedEventFilter) {
@@ -716,35 +722,38 @@
final SkyKey midKey = GraphTester.toSkyKey("mid");
SkyKey badKey = GraphTester.toSkyKey("bad");
final AtomicBoolean waitForSecondCall = new AtomicBoolean(false);
- final TrackingAwaiter trackingAwaiter = new TrackingAwaiter();
final CountDownLatch otherThreadWinning = new CountDownLatch(1);
final AtomicReference<Thread> firstThread = new AtomicReference<>();
- graph = new NotifyingInMemoryGraph(new Listener() {
- @Override
- public void accept(SkyKey key, EventType type, Order order, Object context) {
- if (!waitForSecondCall.get()) {
- return;
- }
- if (key.equals(midKey)) {
- if (type == EventType.CREATE_IF_ABSENT) {
- // The first thread to create midKey will not be the first thread to add a reverse dep
- // to it.
- firstThread.compareAndSet(null, Thread.currentThread());
- return;
- }
- if (type == EventType.ADD_REVERSE_DEP) {
- if (order == Order.BEFORE && Thread.currentThread().equals(firstThread.get())) {
- // If this thread created midKey, block until the other thread adds a dep on it.
- trackingAwaiter.awaitLatchAndTrackExceptions(otherThreadWinning,
- "other thread didn't pass this one");
- } else if (order == Order.AFTER && !Thread.currentThread().equals(firstThread.get())) {
- // This thread has added a dep. Allow the other thread to proceed.
- otherThreadWinning.countDown();
- }
- }
- }
- }
- });
+ graph =
+ new NotifyingInMemoryGraph(
+ new Listener() {
+ @Override
+ public void accept(SkyKey key, EventType type, Order order, Object context) {
+ if (!waitForSecondCall.get()) {
+ return;
+ }
+ if (key.equals(midKey)) {
+ if (type == EventType.CREATE_IF_ABSENT) {
+ // The first thread to create midKey will not be the first thread to add a
+ // reverse dep to it.
+ firstThread.compareAndSet(null, Thread.currentThread());
+ return;
+ }
+ if (type == EventType.ADD_REVERSE_DEP) {
+ if (order == Order.BEFORE && Thread.currentThread().equals(firstThread.get())) {
+ // If this thread created midKey, block until the other thread adds a dep on
+ // it.
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ otherThreadWinning, "other thread didn't pass this one");
+ } else if (order == Order.AFTER
+ && !Thread.currentThread().equals(firstThread.get())) {
+ // This thread has added a dep. Allow the other thread to proceed.
+ otherThreadWinning.countDown();
+ }
+ }
+ }
+ }
+ });
tester.getOrCreate(topKey).addDependency(midKey).setComputedValue(CONCATENATE);
tester.getOrCreate(midKey).addDependency(badKey).setComputedValue(CONCATENATE);
tester.getOrCreate(badKey).setHasError(true);
@@ -752,7 +761,6 @@
assertThat(result.getError(midKey).getRootCauses()).containsExactly(badKey);
waitForSecondCall.set(true);
result = eval(/*keepGoing=*/true, topKey, midKey);
- trackingAwaiter.assertNoErrors();
assertNotNull(firstThread.get());
assertEquals(0, otherThreadWinning.getCount());
assertThat(result.getError(midKey).getRootCauses()).containsExactly(badKey);
@@ -2042,76 +2050,82 @@
final SkyKey otherErrorKey = GraphTester.toSkyKey("otherErrorKey");
final CountDownLatch errorCommitted = new CountDownLatch(1);
- final TrackingAwaiter trackingAwaiterForErrorCommitted = new TrackingAwaiter();
final CountDownLatch otherStarted = new CountDownLatch(1);
- final TrackingAwaiter trackingAwaiterForOtherStarted = new TrackingAwaiter();
final CountDownLatch otherDone = new CountDownLatch(1);
- final TrackingAwaiter trackingAwaiterForOtherDone = new TrackingAwaiter();
final AtomicInteger numOtherInvocations = new AtomicInteger(0);
final AtomicReference<String> bogusInvocationMessage = new AtomicReference<>(null);
final AtomicReference<String> nonNullValueMessage = new AtomicReference<>(null);
- tester.getOrCreate(errorKey).setBuilder(new SkyFunction() {
- @Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
- // Given that errorKey waits for otherErrorKey to begin evaluation before completing its
- // evaluation,
- trackingAwaiterForOtherStarted.awaitLatchAndTrackExceptions(otherStarted,
- "otherErrorKey's SkyFunction didn't start in time.");
- // And given that errorKey throws an error,
- throw new GenericFunctionException(new SomeErrorException("error"), Transience.PERSISTENT);
- }
+ tester
+ .getOrCreate(errorKey)
+ .setBuilder(
+ new SkyFunction() {
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
+ // Given that errorKey waits for otherErrorKey to begin evaluation before completing
+ // its evaluation,
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ otherStarted, "otherErrorKey's SkyFunction didn't start in time.");
+ // And given that errorKey throws an error,
+ throw new GenericFunctionException(
+ new SomeErrorException("error"), Transience.PERSISTENT);
+ }
- @Override
- public String extractTag(SkyKey skyKey) {
- return null;
- }
- });
- tester.getOrCreate(otherErrorKey).setBuilder(new SkyFunction() {
- @Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
- otherStarted.countDown();
- int invocations = numOtherInvocations.incrementAndGet();
- // And given that otherErrorKey waits for errorKey's error to be committed before trying
- // to get errorKey's value,
- trackingAwaiterForErrorCommitted.awaitLatchAndTrackExceptions(errorCommitted,
- "errorKey's error didn't get committed to the graph in time");
- try {
- SkyValue value = env.getValueOrThrow(errorKey, SomeErrorException.class);
- if (value != null) {
- nonNullValueMessage.set("bogus non-null value " + value);
- }
- if (invocations != 1) {
- bogusInvocationMessage.set("bogus invocation count: " + invocations);
- }
- otherDone.countDown();
- // And given that otherErrorKey throws an error,
- throw new GenericFunctionException(new SomeErrorException("other"),
- Transience.PERSISTENT);
- } catch (SomeErrorException e) {
- fail();
- return null;
- }
- }
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ });
+ tester
+ .getOrCreate(otherErrorKey)
+ .setBuilder(
+ new SkyFunction() {
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
+ otherStarted.countDown();
+ int invocations = numOtherInvocations.incrementAndGet();
+ // And given that otherErrorKey waits for errorKey's error to be committed before
+ // trying to get errorKey's value,
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ errorCommitted, "errorKey's error didn't get committed to the graph in time");
+ try {
+ SkyValue value = env.getValueOrThrow(errorKey, SomeErrorException.class);
+ if (value != null) {
+ nonNullValueMessage.set("bogus non-null value " + value);
+ }
+ if (invocations != 1) {
+ bogusInvocationMessage.set("bogus invocation count: " + invocations);
+ }
+ otherDone.countDown();
+ // And given that otherErrorKey throws an error,
+ throw new GenericFunctionException(
+ new SomeErrorException("other"), Transience.PERSISTENT);
+ } catch (SomeErrorException e) {
+ fail();
+ return null;
+ }
+ }
- @Override
- public String extractTag(SkyKey skyKey) {
- return null;
- }
- });
- graph = new NotifyingInMemoryGraph(new Listener() {
- @Override
- public void accept(SkyKey key, EventType type, Order order, Object context) {
- if (key.equals(errorKey) && type == EventType.SET_VALUE && order == Order.AFTER) {
- errorCommitted.countDown();
- trackingAwaiterForOtherDone.awaitLatchAndTrackExceptions(otherDone,
- "otherErrorKey's SkyFunction didn't finish in time.");
- }
- }
- });
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ });
+ graph =
+ new NotifyingInMemoryGraph(
+ new Listener() {
+ @Override
+ public void accept(SkyKey key, EventType type, Order order, Object context) {
+ if (key.equals(errorKey) && type == EventType.SET_VALUE && order == Order.AFTER) {
+ errorCommitted.countDown();
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ otherDone, "otherErrorKey's SkyFunction didn't finish in time.");
+ }
+ }
+ });
// When the graph is evaluated in noKeepGoing mode,
EvaluationResult<StringValue> result = eval(/*keepGoing=*/false,
@@ -2142,11 +2156,8 @@
@Test
public void raceConditionWithNoKeepGoingErrors_FutureError() throws Exception {
final CountDownLatch errorCommitted = new CountDownLatch(1);
- final TrackingAwaiter trackingAwaiterForError = new TrackingAwaiter();
final CountDownLatch otherStarted = new CountDownLatch(1);
- final TrackingAwaiter trackingAwaiterForOther = new TrackingAwaiter();
final CountDownLatch otherParentSignaled = new CountDownLatch(1);
- final TrackingAwaiter trackingAwaiterForOtherParent = new TrackingAwaiter();
final SkyKey errorParentKey = GraphTester.toSkyKey("errorParentKey");
final SkyKey errorKey = GraphTester.toSkyKey("errorKey");
final SkyKey otherParentKey = GraphTester.toSkyKey("otherParentKey");
@@ -2166,34 +2177,40 @@
return null;
}
});
- tester.getOrCreate(otherKey).setBuilder(new SkyFunction() {
- @Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
- otherStarted.countDown();
- trackingAwaiterForError.awaitLatchAndTrackExceptions(errorCommitted,
- "error didn't get committed to the graph in time");
- return new StringValue("other");
- }
+ tester
+ .getOrCreate(otherKey)
+ .setBuilder(
+ new SkyFunction() {
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
+ otherStarted.countDown();
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ errorCommitted, "error didn't get committed to the graph in time");
+ return new StringValue("other");
+ }
- @Override
- public String extractTag(SkyKey skyKey) {
- return null;
- }
- });
- tester.getOrCreate(errorKey).setBuilder(new SkyFunction() {
- @Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
- trackingAwaiterForOther.awaitLatchAndTrackExceptions(otherStarted,
- "other didn't start in time");
- throw new GenericFunctionException(new SomeErrorException("error"),
- Transience.PERSISTENT);
- }
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ });
+ tester
+ .getOrCreate(errorKey)
+ .setBuilder(
+ new SkyFunction() {
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ otherStarted, "other didn't start in time");
+ throw new GenericFunctionException(
+ new SomeErrorException("error"), Transience.PERSISTENT);
+ }
- @Override
- public String extractTag(SkyKey skyKey) {
- return null;
- }
- });
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ });
tester.getOrCreate(errorParentKey).setBuilder(new SkyFunction() {
@Override
public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException {
@@ -2221,26 +2238,31 @@
return null;
}
});
- graph = new NotifyingInMemoryGraph(new Listener() {
- @Override
- public void accept(SkyKey key, EventType type, Order order, Object context) {
- if (key.equals(errorKey) && type == EventType.SET_VALUE && order == Order.AFTER) {
- errorCommitted.countDown();
- trackingAwaiterForOtherParent.awaitLatchAndTrackExceptions(otherParentSignaled,
- "otherParent didn't get signaled in time");
- // We try to give some time for ParallelEvaluator to incorrectly re-evaluate
- // 'otherParentKey'. This test case is testing for a real race condition and the 10ms time
- // was chosen experimentally to give a true positive rate of 99.8% (without a sleep it
- // has a 1% true positive rate). There's no good way to do this without sleeping. We
- // *could* introspect ParallelEvaulator's AbstractQueueVisitor to see if the re-evaluation
- // has been enqueued, but that's relying on pretty low-level implementation details.
- Uninterruptibles.sleepUninterruptibly(10, TimeUnit.MILLISECONDS);
- }
- if (key.equals(otherParentKey) && type == EventType.SIGNAL && order == Order.AFTER) {
- otherParentSignaled.countDown();
- }
- }
- });
+ graph =
+ new NotifyingInMemoryGraph(
+ new Listener() {
+ @Override
+ public void accept(SkyKey key, EventType type, Order order, Object context) {
+ if (key.equals(errorKey) && type == EventType.SET_VALUE && order == Order.AFTER) {
+ errorCommitted.countDown();
+ TrackingAwaiter.INSTANCE.awaitLatchAndTrackExceptions(
+ otherParentSignaled, "otherParent didn't get signaled in time");
+ // We try to give some time for ParallelEvaluator to incorrectly re-evaluate
+ // 'otherParentKey'. This test case is testing for a real race condition and the
+ // 10ms time was chosen experimentally to give a true positive rate of 99.8%
+ // (without a sleep it has a 1% true positive rate). There's no good way to do
+ // this without sleeping. We *could* introspect ParallelEvaulator's
+ // AbstractQueueVisitor to see if the re-evaluation has been enqueued, but that's
+ // relying on pretty low-level implementation details.
+ Uninterruptibles.sleepUninterruptibly(10, TimeUnit.MILLISECONDS);
+ }
+ if (key.equals(otherParentKey)
+ && type == EventType.SIGNAL
+ && order == Order.AFTER) {
+ otherParentSignaled.countDown();
+ }
+ }
+ });
EvaluationResult<StringValue> result = eval(/*keepGoing=*/false,
ImmutableList.of(otherParentKey, errorParentKey));
assertTrue(result.hasError());
diff --git a/src/test/java/com/google/devtools/build/skyframe/TrackingAwaiter.java b/src/test/java/com/google/devtools/build/skyframe/TrackingAwaiter.java
index 3757583..687ed9a 100644
--- a/src/test/java/com/google/devtools/build/skyframe/TrackingAwaiter.java
+++ b/src/test/java/com/google/devtools/build/skyframe/TrackingAwaiter.java
@@ -24,8 +24,16 @@
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
-/** Safely await {@link CountDownLatch}es in tests, storing any exceptions that happen. */
+/**
+ * Safely await {@link CountDownLatch}es in tests, storing any exceptions that happen. Callers
+ * should call {@link #assertNoErrors} at the end of each test method, either manually or using an
+ * {@code @After} hook.
+ */
public class TrackingAwaiter {
+ public static final TrackingAwaiter INSTANCE = new TrackingAwaiter();
+
+ private TrackingAwaiter() {}
+
private final ConcurrentLinkedQueue<Pair<String, Throwable>> exceptionsThrown =
new ConcurrentLinkedQueue<>();
@@ -42,7 +50,7 @@
* this was not a race condition, but an honest-to-goodness interrupt, and we propagate the
* exception onward.
*/
- public static void waitAndMaybeThrowInterrupt(CountDownLatch latch, String errorMessage)
+ private static void waitAndMaybeThrowInterrupt(CountDownLatch latch, String errorMessage)
throws InterruptedException {
if (Uninterruptibles.awaitUninterruptibly(latch, TestUtils.WAIT_TIMEOUT_SECONDS,
TimeUnit.SECONDS)) {
