Automatically record exceptions and assert that there aren't any when using NotifyingInMemoryGraph in tests.

--
MOS_MIGRATED_REVID=102616906

3 files changed, 86 insertions, 79 deletions

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 7d7cebeafc..166ae24b4e 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -46,7 +46,6 @@ import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.testutil.TestThread;
 import com.google.devtools.build.lib.testutil.TestUtils;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.skyframe.GraphTester.StringValue;
 import com.google.devtools.build.skyframe.GraphTester.TestFunction;
 import com.google.devtools.build.skyframe.GraphTester.ValueComputer;
@@ -72,7 +71,6 @@ import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicReference;
 
 import javax.annotation.Nullable;
 
@@ -86,6 +84,7 @@ public class MemoizingEvaluatorTest {
   private EventCollector eventCollector;
   private EventHandler reporter;
   protected MemoizingEvaluator.EmittedEventState emittedEventState;
+  @Nullable NotifyingInMemoryGraph graph = null;
 
   // Knobs that control the size / duration of larger tests.
   private static final int TEST_NODE_COUNT = 100;
@@ -100,6 +99,9 @@ public class MemoizingEvaluatorTest {
   @After
   public void assertNoTrackedErrors() {
     TrackingAwaiter.INSTANCE.assertNoErrors();
+    if (graph != null) {
+      graph.assertNoExceptions();
+    }
   }
 
   public void initializeTester(@Nullable TrackingInvalidationReceiver customInvalidationReceiver) {
@@ -2941,50 +2943,45 @@ public class MemoizingEvaluatorTest {
     final AtomicBoolean synchronizeThreads = new AtomicBoolean(false);
     // We don't expect slow-set-value to actually be built, but if it is, we wait for it.
     final CountDownLatch slowBuilt = new CountDownLatch(1);
-    // Keep track of any exceptions thrown during evaluation.
-    final AtomicReference<Pair<SkyKey, ? extends Exception>> unexpectedException =
-        new AtomicReference<>();
-    setGraphForTesting(new DeterministicInMemoryGraph(new Listener() {
-      @Override
-      public void accept(SkyKey key, EventType type, Order order, Object context) {
-        if (!synchronizeThreads.get()) {
-          return;
-        }
-        if (type == EventType.IS_DIRTY && key.equals(failingKey)) {
-          // Wait for the build to abort or for the other node to incorrectly build.
-          try {
-            assertTrue(slowBuilt.await(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS));
-          } catch (InterruptedException e) {
-            // This is ok, because it indicates the build is shutting down.
-            Thread.currentThread().interrupt();
-          }
-        } else if (type == EventType.SET_VALUE && key.equals(fastToRequestSlowToSetValueKey)
-            && order == Order.AFTER) {
-          // This indicates a problem -- this parent shouldn't be built since it depends on an
-          // error.
-          slowBuilt.countDown();
-          // Before this node actually sets its value (and then throws an exception) we wait for the
-          // other node to throw an exception.
-          try {
-            Thread.sleep(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
-            unexpectedException.set(Pair.of(key, new IllegalStateException("uninterrupted")));
-          } catch (InterruptedException e) {
-            Thread.currentThread().interrupt();
-          }
-        }
-      }
-    }));
+    setGraphForTesting(
+        new DeterministicInMemoryGraph(
+            new Listener() {
+              @Override
+              public void accept(SkyKey key, EventType type, Order order, Object context) {
+                if (!synchronizeThreads.get()) {
+                  return;
+                }
+                if (type == EventType.IS_DIRTY && key.equals(failingKey)) {
+                  // Wait for the build to abort or for the other node to incorrectly build.
+                  try {
+                    assertTrue(slowBuilt.await(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS));
+                  } catch (InterruptedException e) {
+                    // This is ok, because it indicates the build is shutting down.
+                    Thread.currentThread().interrupt();
+                  }
+                } else if (type == EventType.SET_VALUE
+                    && key.equals(fastToRequestSlowToSetValueKey)
+                    && order == Order.AFTER) {
+                  // This indicates a problem -- this parent shouldn't be built since it depends on
+                  // an error.
+                  slowBuilt.countDown();
+                  // Before this node actually sets its value (and then throws an exception) we wait
+                  // for the other node to throw an exception.
+                  try {
+                    Thread.sleep(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
+                    throw new IllegalStateException("uninterrupted in " + key);
+                  } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();
+                  }
+                }
+              }
+            }));
     // Initialize graph.
     tester.eval(/*keepGoing=*/true, errorKey);
     tester.getOrCreate(invalidatedKey, /*markAsModified=*/true);
     tester.invalidate();
     synchronizeThreads.set(true);
     tester.eval(/*keepGoing=*/false, fastToRequestSlowToSetValueKey, failingKey);
-    Pair<SkyKey, ? extends Exception> unexpected = unexpectedException.get();
-    if (unexpected != null) {
-      throw new AssertionError(unexpected.first + ", " + unexpected.second + ", "
-          + Arrays.toString(unexpected.second.getStackTrace()));
-    }
   }
 
   /**
@@ -3018,9 +3015,6 @@ public class MemoizingEvaluatorTest {
     // particular, we don't want to force anything during error bubbling.
     final AtomicBoolean synchronizeThreads = new AtomicBoolean(false);
     final CountDownLatch shutdownAwaiterStarted = new CountDownLatch(1);
-    // Keep track of any exceptions thrown during evaluation.
-    final AtomicReference<Pair<SkyKey, ? extends Exception>> unexpectedException =
-        new AtomicReference<>();
     setGraphForTesting(
         new DeterministicInMemoryGraph(
             new Listener() {
@@ -3039,22 +3033,8 @@ public class MemoizingEvaluatorTest {
                   // When the uncached parent is first signaled by its changed dep, make sure that
                   // we wait until the cached parent is signaled too.
                   try {
-                    if (!cachedSignaled.await(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
-                      Thread currentThread = Thread.currentThread();
-                      unexpectedException.set(
-                          Pair.of(
-                              key,
-                              new Exception(
-                                  "no interruption or signaling in time for "
-                                      + (currentThread.isInterrupted() ? "" : "un")
-                                      + "interrupted "
-                                      + currentThread
-                                      + " with hash "
-                                      + System.identityHashCode(currentThread)
-                                      + " at "
-                                      + System.currentTimeMillis())));
-                      return;
-                    }
+                    assertTrue(
+                        cachedSignaled.await(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS));
                   } catch (InterruptedException e) {
                     // Before the relevant bug was fixed, this code was not interrupted, and the
                     // uncached parent got to build, yielding an inconsistent state at a later point
@@ -3073,19 +3053,17 @@ public class MemoizingEvaluatorTest {
                     // down.
                     Thread.sleep(TestUtils.WAIT_TIMEOUT_MILLISECONDS);
                     Thread currentThread = Thread.currentThread();
-                    unexpectedException.set(
-                        Pair.of(
-                            key,
-                            new Exception(
-                                "no interruption in time for "
-                                    + (currentThread.isInterrupted() ? "" : "un")
-                                    + "interrupted "
-                                    + currentThread
-                                    + " with hash "
-                                    + System.identityHashCode(currentThread)
-                                    + " at "
-                                    + System.currentTimeMillis())));
-                    return;
+                    throw new IllegalStateException(
+                        "no interruption in time in "
+                            + key
+                            + " for "
+                            + (currentThread.isInterrupted() ? "" : "un")
+                            + "interrupted "
+                            + currentThread
+                            + " with hash "
+                            + System.identityHashCode(currentThread)
+                            + " at "
+                            + System.currentTimeMillis());
                   } catch (InterruptedException e) {
                     Thread.currentThread().interrupt();
                   }
@@ -3124,11 +3102,6 @@ public class MemoizingEvaluatorTest {
     EvaluationResult<StringValue> result =
         tester.eval(/*keepGoing=*/false, cachedParentKey, uncachedParentKey, waitForShutdownKey);
     assertWithMessage(result.toString()).that(result.hasError()).isTrue();
-    Pair<SkyKey, ? extends Exception> unexpected = unexpectedException.get();
-    if (unexpected != null) {
-      throw new AssertionError(unexpected.first + ", " + unexpected.second + ", "
-          + Arrays.toString(unexpected.second.getStackTrace()));
-    }
     tester.getOrCreate(invalidatedKey, /*markAsModified=*/true);
     tester.invalidate();
     result = tester.eval(/*keepGoing=*/false, cachedParentKey, uncachedParentKey);
@@ -3170,6 +3143,7 @@ public class MemoizingEvaluatorTest {
   }
 
   private void setGraphForTesting(NotifyingInMemoryGraph notifyingInMemoryGraph) {
+    graph = notifyingInMemoryGraph;
     InMemoryMemoizingEvaluator memoizingEvaluator = (InMemoryMemoizingEvaluator) tester.evaluator;
     memoizingEvaluator.setGraphForTesting(notifyingInMemoryGraph);
   }
diff --git a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
index 0adaabb339..53f6ebffa5 100644
--- a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
+++ b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
@@ -13,19 +13,24 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
+import com.google.common.truth.Truth;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 
+import java.util.ArrayList;
 import java.util.Set;
 
 /**
  * Class that allows clients to be notified on each access of the graph. Clients can simply track
- * accesses, or they can block to achieve desired synchronization.
+ * accesses, or they can block to achieve desired synchronization. Clients should call
+ * {@link #assertNoExceptions} at the end of tests in case exceptions were swallowed in async
+ * threads.
  */
 public class NotifyingInMemoryGraph extends InMemoryGraph {
   private final Listener graphListener;
+  private final ArrayList<Exception> unexpectedExceptions = new ArrayList<>();
 
   public NotifyingInMemoryGraph(Listener graphListener) {
-    this.graphListener = graphListener;
+    this.graphListener = new ErrorRecordingDelegatingListener(graphListener);
   }
 
   @Override
@@ -36,6 +41,14 @@ public class NotifyingInMemoryGraph extends InMemoryGraph {
     return oldval == null ? newval : oldval;
   }
 
+  /**
+   * Should be called at end of test (ideally in an {@code @After} method) to assert that no
+   * exceptions were thrown during calls to the listener.
+   */
+  public void assertNoExceptions() {
+    Truth.assertThat(unexpectedExceptions).isEmpty();
+  }
+
   // Subclasses should override if they wish to subclass NotifyingNodeEntry.
   protected NotifyingNodeEntry getEntry(SkyKey key) {
     return new NotifyingNodeEntry(key);
@@ -52,6 +65,23 @@ public class NotifyingInMemoryGraph extends InMemoryGraph {
     };
   }
 
+  private class ErrorRecordingDelegatingListener implements Listener {
+    private final Listener delegate;
+
+    private ErrorRecordingDelegatingListener(Listener delegate) {
+      this.delegate = delegate;
+    }
+
+    @Override
+    public void accept(SkyKey key, EventType type, Order order, Object context) {
+      try {
+        delegate.accept(key, type, order, context);
+      } catch (Exception e) {
+        unexpectedExceptions.add(e);
+        throw e;
+      }
+    }
+  }
   /**
    * Graph/value entry events that the receiver can be informed of. When writing tests, feel free to
    * add additional events here if needed.
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 3969dfaee1..fc675b57ea 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -97,6 +97,9 @@ public class ParallelEvaluatorTest {
   @After
   public void assertNoTrackedErrors() {
     TrackingAwaiter.INSTANCE.assertNoErrors();
+    if (graph instanceof NotifyingInMemoryGraph) {
+      ((NotifyingInMemoryGraph) graph).assertNoExceptions();
+    }
   }
 
   private ParallelEvaluator makeEvaluator(ProcessableGraph graph,