commit 1c121708adf831088a7773cb255f184409f252c2
author Googler <noreply@google.com> Thu Nov 01 14:04:09 2018 -0700
committer Copybara-Service <copybara-piper@google.com> Thu Nov 01 14:05:49 2018 -0700
tree 6f4d026eec3f5b969e48a94327f57bea54af2003
parent 3def8c4fe75365ed27009fc15d4431720b14335b

Fix incorrect ChangedValueAtSameVersionException.

I had moved up the setting of this.value so that it would be part of the logged error. However, setting this.value prior to this.lastChangedVersion may lead to a race condition, so exceptions are being thrown when they shouldn't be.

I will attempt to follow up with a threaded test to catch this.

RELNOTES: None.
PiperOrigin-RevId: 219694529

src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java

diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
index 1ff6fe3..ad7105a 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
@@ -291,6 +291,9 @@
     return (Iterable<SkyKey>) (List<?>) reverseDepsDataToConsolidate;
   }
 
+  // In this method it is critical that this.lastChangedVersion is set prior to this.value because
+  // although this method itself is synchronized, there are unsynchronized consumers of the version
+  // and the value.
   @Override
   public synchronized Set<SkyKey> setValue(SkyValue value, Version version)
       throws InterruptedException {
@@ -306,15 +309,16 @@
       // value, because preserving == equality is even better than .equals() equality.
       this.value = getDirtyBuildingState().getLastBuildValue();
     } else {
-      this.value = value;
       boolean forcedRebuild =
           isDirty() && getDirtyBuildingState().getDirtyState() == DirtyState.FORCED_REBUILDING;
       if (!forcedRebuild && this.lastChangedVersion.equals(version)) {
-        logError(new ChangedValueAtSameVersionException(this.lastChangedVersion, version, this));
+        logError(
+            new ChangedValueAtSameVersionException(this.lastChangedVersion, version, value, this));
       }
       // If this is a new value, or it has changed since the last build, set the version to the
       // current graph version.
       this.lastChangedVersion = version;
+      this.value = value;
     }
     return setStateFinishedAndReturnReverseDepsToSignal();
   }
@@ -345,11 +349,15 @@
   /** An exception indicating that the node's value changed but its version did not. */
   public static final class ChangedValueAtSameVersionException extends IllegalStateException {
     private ChangedValueAtSameVersionException(
-        Version lastChangedVersion, Version newVersion, InMemoryNodeEntry nodeEntry) {
+        Version lastChangedVersion,
+        Version newVersion,
+        SkyValue newValue,
+        InMemoryNodeEntry nodeEntry) {
       super(
           String.format(
-              "Changed value but with the same version? %s %s %s",
-              lastChangedVersion, newVersion, nodeEntry));
+              "Changed value but with the same version? "
+                  + "lastChangedVersion: %s, newVersion: %s newValue: %s, nodeEntry: %s",
+              lastChangedVersion, newVersion, newValue, nodeEntry));
     }
   }