diff options
author | Janak Ramakrishnan <janakr@google.com> | 2015-09-02 17:56:08 +0000 |
---|---|---|
committer | Florian Weikert <fwe@google.com> | 2015-09-02 21:08:04 +0000 |
commit | a67bb8b2ad5506c2a3c7c4476d9878d4f916656d (patch) | |
tree | b01d6c99919f3dc5882115da32e477384098c52f /src/test/java/com/google/devtools/build | |
parent | 2e003439cecfe87e318037f19ffd5e3761724948 (diff) |
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
Diffstat (limited to 'src/test/java/com/google/devtools/build')
5 files changed, 332 insertions, 294 deletions
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 4f87bb38ca..48325b0765 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 @@ final class ChainedFunction implements SkyFunction { 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 c0d349d025..aa9b036154 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.DirtyingNodeVi 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 @@ public class EagerInvalidatorTest { 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 @@ public class EagerInvalidatorTest { } 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 9199204349..c75af925a6 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.Listener; 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 @@ public class MemoizingEvaluatorTest { 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 class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { || 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { 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 5d90a50e61..4ca65506f5 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.Listener; 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 @@ public class ParallelEvaluatorTest { 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 @@ public class ParallelEvaluatorTest { 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 @@ public class ParallelEvaluatorTest { 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 @@ public class ParallelEvaluatorTest { 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); - } - - @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; - } - }); - 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."); - } - } - }); + 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, + 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(); + 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 @@ public class ParallelEvaluatorTest { @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 @@ public class ParallelEvaluatorTest { 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"); - } - - @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(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 { + 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; + } + }); tester.getOrCreate(errorParentKey).setBuilder(new SkyFunction() { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException { @@ -2221,26 +2238,31 @@ public class ParallelEvaluatorTest { 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 3757583ccb..687ed9a1d9 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.ConcurrentLinkedQueue; 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 @@ public class TrackingAwaiter { * 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)) { |