Mark nodes to be force-rebuilt during node restarting
Non-deterministic nodes may evaluate to different values after being
restarted. By ensuring that the nodes are "force-rebuilt" after a
restart, such a situation won't result in a crash.
Because restarted nodes are no longer dirtied with isChanged=true, the
assertion logic that ensures that nodes are not dirtied twice with the
same value for isChanged can go back into the #markDirty method. In
other words, this CL includes a logical rollback of "Permit marking
dirty/changed a node more than once".
This also fixes a race condition bug where a dep which is restarted by
>1 parent could cause a crash if the dep was inflight when the later
restart attempted to dirty it.
RELNOTES: None.
PiperOrigin-RevId: 214954192
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 2a0f664..25b867f 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
@@ -475,27 +475,35 @@
}
@Override
- public synchronized MarkedDirtyResult markDirty(boolean isChanged) {
+ public synchronized MarkedDirtyResult markDirty(DirtyType dirtyType) {
// Can't process a dirty node without its deps.
assertKeepDeps();
if (isDone()) {
dirtyBuildingState =
- DirtyBuildingState.create(isChanged, GroupedList.<SkyKey>create(directDeps), value);
+ DirtyBuildingState.create(dirtyType, GroupedList.create(directDeps), value);
value = null;
directDeps = null;
- return new FromCleanMarkedDirtyResult(ReverseDepsUtility.getReverseDeps(this));
+ return new MarkedDirtyResult(ReverseDepsUtility.getReverseDeps(this));
}
-
+ if (dirtyType.equals(DirtyType.FORCE_REBUILD)) {
+ getDirtyBuildingState().markForceRebuild();
+ return null;
+ }
+ // The caller may be simultaneously trying to mark this node dirty and changed, and the dirty
+ // thread may have lost the race, but it is the caller's responsibility not to try to mark
+ // this node changed twice. The end result of racing markers must be a changed node, since one
+ // of the markers is trying to mark the node changed.
+ Preconditions.checkState(
+ dirtyType.equals(DirtyType.CHANGE) != isChanged(),
+ "Cannot mark node dirty twice or changed twice: %s",
+ this);
Preconditions.checkState(value == null, "Value should have been reset already %s", this);
- if (isChanged != isChanged()) {
- if (isChanged) {
- getDirtyBuildingState().markChanged();
- }
- // If !isChanged, then this call made no changes to the node, but redundancy is a property of
- // the sequence of markDirty calls, not their effects.
- return FromDirtyMarkedDirtyResult.NOT_REDUNDANT;
+ if (dirtyType.equals(DirtyType.CHANGE)) {
+ // If the changed marker lost the race, we just need to mark changed in this method -- all
+ // other work was done by the dirty marker.
+ getDirtyBuildingState().markChanged();
}
- return FromDirtyMarkedDirtyResult.REDUNDANT;
+ return null;
}
@Override
@@ -514,7 +522,7 @@
@Override
public synchronized void forceRebuild() {
Preconditions.checkState(getNumTemporaryDirectDeps() == signaledDeps, this);
- getDirtyBuildingState().forceChanged();
+ getDirtyBuildingState().forceRebuild();
}
@Override
