Singleton-ify ErrorTransienceValue

We rely on ErrorTransienceValue being different across evaluations by
way of java reference inequality, instead make it a singleton and make
it so that ErrorTransienceValue compares equal to nothing, including itself.

Alternatively we could attach the corresponding Version to ErrorTransienceValue
and use that to force inequality across evaluations. However this gets into
the different possible Version implementations and their varying semantics, so
go with the simplest approach first and see how it works out.

--
MOS_MIGRATED_REVID=106001826

5 files changed, 109 insertions, 18 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
index 0ac2b39f35..3d5ede4b11 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
@@ -13,16 +13,44 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+
 /**
  * A value that represents "error transience", i.e. anything which may have caused an unexpected
- * failure.
+ * failure. Is not equal to anything, including itself, in order to force re-evaluation.
  */
 public final class ErrorTransienceValue implements SkyValue {
   public static final SkyFunctionName FUNCTION_NAME = SkyFunctionName.create("ERROR_TRANSIENCE");
+  public static final SkyKey KEY = new SkyKey(FUNCTION_NAME, "ERROR_TRANSIENCE");
+  public static final ErrorTransienceValue INSTANCE = new ErrorTransienceValue();
+
+  private ErrorTransienceValue() {}
 
-  ErrorTransienceValue() {}
+  @Override
+  public int hashCode() {
+    // Not the prettiest, but since we always return false for equals throw exception here to
+    // catch any errors related to hash-based collections quickly.
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public boolean equals(Object other) {
+    return false;
+  }
+
+  @Override
+  public String toString() {
+    return "ErrorTransienceValue";
+  }
+
+  @SuppressWarnings("unused")
+  private void writeObject(ObjectOutputStream unused) {
+    throw new UnsupportedOperationException("Java serialization not supported");
+  }
 
-  public static SkyKey key() {
-    return new SkyKey(FUNCTION_NAME, "ERROR_TRANSIENCE");
+  @SuppressWarnings("unused")
+  private void readObject(ObjectInputStream unused) {
+    throw new UnsupportedOperationException("Java serialization not supported");
   }
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
index a7c7aee204..6f2bc9d3c6 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -334,13 +334,13 @@ public final class ParallelEvaluator implements Evaluator {
 
       if (errorInfo.isTransient()) {
         DependencyState triState =
-            graph.get(ErrorTransienceValue.key()).addReverseDepAndCheckIfDone(skyKey);
+            graph.get(ErrorTransienceValue.KEY).addReverseDepAndCheckIfDone(skyKey);
         Preconditions.checkState(triState == DependencyState.DONE,
             "%s %s %s", skyKey, triState, errorInfo);
 
         final NodeEntry state = graph.get(skyKey);
         state.addTemporaryDirectDeps(
-            GroupedListHelper.create(ImmutableList.of(ErrorTransienceValue.key())));
+            GroupedListHelper.create(ImmutableList.of(ErrorTransienceValue.KEY)));
         state.signalDep();
       }
 
@@ -387,7 +387,7 @@ public final class ParallelEvaluator implements Evaluator {
       Map<SkyKey, ValueWithMetadata> values = getValuesMaybeFromError(depKeys, bubbleErrorInfo);
       ImmutableMap.Builder<SkyKey, ValueOrUntypedException> builder = ImmutableMap.builder();
       for (SkyKey depKey : depKeys) {
-        Preconditions.checkState(!depKey.equals(ErrorTransienceValue.key()));
+        Preconditions.checkState(!depKey.equals(ErrorTransienceValue.KEY));
         ValueWithMetadata value = values.get(depKey);
         if (value == null) {
           // If this entry is not yet done then (optionally) record the missing dependency and
@@ -747,8 +747,8 @@ public final class ParallelEvaluator implements Evaluator {
      */
     private boolean invalidatedByErrorTransience(Collection<SkyKey> depGroup, NodeEntry entry) {
       return depGroup.size() == 1
-          && depGroup.contains(ErrorTransienceValue.key())
-          && !graph.get(ErrorTransienceValue.key()).getVersion().atMost(entry.getVersion());
+          && depGroup.contains(ErrorTransienceValue.KEY)
+          && !graph.get(ErrorTransienceValue.KEY).getVersion().atMost(entry.getVersion());
     }
 
     private DirtyOutcome maybeHandleDirtyNode(NodeEntry state) {
@@ -782,7 +782,7 @@ public final class ParallelEvaluator implements Evaluator {
             // usual, but we can't, because then the ErrorTransienceValue would remain as a dep,
             // which would be incorrect if, for instance, the value re-evaluated to a non-error.
             state.forceRebuild();
-            graph.get(ErrorTransienceValue.key()).removeReverseDep(skyKey);
+            graph.get(ErrorTransienceValue.KEY).removeReverseDep(skyKey);
             return DirtyOutcome.NEEDS_EVALUATION;
           }
           if (!keepGoing) {
@@ -872,7 +872,7 @@ public final class ParallelEvaluator implements Evaluator {
       // direct deps that were requested on a previous run. This would allow us to avoid the
       // conversion of the direct deps into a set.
       Set<SkyKey> directDeps = state.getTemporaryDirectDeps();
-      Preconditions.checkState(!directDeps.contains(ErrorTransienceValue.key()),
+      Preconditions.checkState(!directDeps.contains(ErrorTransienceValue.KEY),
           "%s cannot have a dep on ErrorTransienceValue during building: %s", skyKey, state);
       // Get the corresponding SkyFunction and call it on this value.
       SkyFunctionEnvironment env = new SkyFunctionEnvironment(skyKey, directDeps, visitor);
@@ -1174,12 +1174,11 @@ public final class ParallelEvaluator implements Evaluator {
     // We unconditionally add the ErrorTransienceValue here, to ensure that it will be created, and
     // in the graph, by the time that it is needed. Creating it on demand in a parallel context sets
     // up a race condition, because there is no way to atomically create a node and set its value.
-    SkyKey errorTransienceKey = ErrorTransienceValue.key();
     NodeEntry errorTransienceEntry = Iterables.getOnlyElement(
-        graph.createIfAbsentBatch(ImmutableList.of(errorTransienceKey)).values());
+        graph.createIfAbsentBatch(ImmutableList.of(ErrorTransienceValue.KEY)).values());
     if (!errorTransienceEntry.isDone()) {
       injectValues(
-          ImmutableMap.of(errorTransienceKey, (SkyValue) new ErrorTransienceValue()),
+          ImmutableMap.of(ErrorTransienceValue.KEY, (SkyValue) ErrorTransienceValue.INSTANCE),
           graphVersion,
           graph,
           dirtyKeyTracker);
diff --git a/src/main/java/com/google/devtools/build/skyframe/RecordingDifferencer.java b/src/main/java/com/google/devtools/build/skyframe/RecordingDifferencer.java
index ffeeb66c21..1582e52b78 100644
--- a/src/main/java/com/google/devtools/build/skyframe/RecordingDifferencer.java
+++ b/src/main/java/com/google/devtools/build/skyframe/RecordingDifferencer.java
@@ -63,7 +63,7 @@ public class RecordingDifferencer implements Differencer, Injectable {
   public void invalidateTransientErrors() {
     // All transient error values have a dependency on the single global ERROR_TRANSIENCE value,
     // so we only have to invalidate that one value to catch everything.
-    invalidate(ImmutableList.of(ErrorTransienceValue.key()));
+    invalidate(ImmutableList.of(ErrorTransienceValue.KEY));
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/skyframe/ErrorTransienceValueTest.java b/src/test/java/com/google/devtools/build/skyframe/ErrorTransienceValueTest.java
new file mode 100644
index 0000000000..64e61a9e7f
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/skyframe/ErrorTransienceValueTest.java
@@ -0,0 +1,64 @@
+// Copyright 2015 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.skyframe;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectOutputStream;
+
+/** Tests for {@link ErrorTransienceValue}. */
+@RunWith(JUnit4.class)
+public class ErrorTransienceValueTest {
+
+  @Test
+  public void testNotSerializable() throws IOException {
+    ObjectOutputStream objOut = new ObjectOutputStream(new ByteArrayOutputStream());
+
+    try {
+      objOut.writeObject(ErrorTransienceValue.INSTANCE);
+      fail("Expected exception");
+    } catch (UnsupportedOperationException e) {
+      assertThat(e).hasMessage("Java serialization not supported");
+    }
+  }
+
+  @Test
+  public void testHashCodeNotSupported() {
+    try {
+      ErrorTransienceValue.INSTANCE.hashCode();
+      fail("Expected exception");
+    } catch (UnsupportedOperationException e) {
+      // Expected.
+    }
+  }
+
+  @Test
+  public void testToStringWorks() {
+    assertThat(ErrorTransienceValue.INSTANCE.toString()).isEqualTo("ErrorTransienceValue");
+  }
+
+  @Test
+  public void testIsntEqualToSelf() {
+    // assertThat(...).isNotEqualTo(...) does ref equality check, so use equals explicitly.
+    assertThat(ErrorTransienceValue.INSTANCE.equals(ErrorTransienceValue.INSTANCE)).isFalse();
+  }
+}
+
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 6e1efebde3..aa554b49e5 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -330,7 +330,7 @@ public class MemoizingEvaluatorTest {
 
     // The graph now contains the three above nodes (and ERROR_TRANSIENCE).
     assertThat(tester.evaluator.getValues().keySet())
-        .containsExactly(skyKey("top"), skyKey("d1"), skyKey("d2"), ErrorTransienceValue.key());
+        .containsExactly(skyKey("top"), skyKey("d1"), skyKey("d2"), ErrorTransienceValue.KEY);
 
     String[] noKeys = {};
     tester.evaluator.deleteDirty(2);
@@ -338,14 +338,14 @@ public class MemoizingEvaluatorTest {
 
     // The top node's value is dirty, but less than two generations old, so it wasn't deleted.
     assertThat(tester.evaluator.getValues().keySet())
-        .containsExactly(skyKey("top"), skyKey("d1"), skyKey("d2"), ErrorTransienceValue.key());
+        .containsExactly(skyKey("top"), skyKey("d1"), skyKey("d2"), ErrorTransienceValue.KEY);
 
     tester.evaluator.deleteDirty(2);
     tester.eval(true, noKeys);
 
     // The top node's value was dirty, and was two generations old, so it was deleted.
     assertThat(tester.evaluator.getValues().keySet())
-        .containsExactly(skyKey("d1"), skyKey("d2"), ErrorTransienceValue.key());
+        .containsExactly(skyKey("d1"), skyKey("d2"), ErrorTransienceValue.KEY);
   }
 
   @Test