Move the initialization of CriticalPathComputer forward to prevent a race
condition in Skymeld mode.
We observed the following race condition in Skymeld:
1. [Thread 1] Target `//foo` starts executing, and sends out
`TopLevelTargetPendingExecutionEvent`
2. [Thread 2] Target `//bar` does the same thing. Its `TopLevelTargetPendingExecutionEvent` is ignored since it arrived after `//foo`'s.
3. [Thread 2] Action `actionBar` starts executing and an `ActionStartedEvent` is posted
4. [Thread 1] `BuildSummaryStatsModule` initializes `CriticalPathComputer` upon receiving `TopLevelTargetPendingExecutionEvent`
5. [Thread 2] Action `actionBar` finishes executing and a `SpawnExecutedEvent` is posted
Between 3 and 4, the `ActionStartedEvent` is "lost", in the sense that it never
reached its intended subscriber `CriticalPathComputer` since the CPC wasn't
initialized yet. When `actionBar` eventually finishes executing and sends out a
`SpawnExecutedEvent`, CPC is left in an illegal state: we have no corresponding
`CriticalPathComponent` for `actionBar`.
This CL fixes that by moving the initialization of CPC to `executorInit`. This is fine: we don't need any information that is only available
right before execution to initialize CPC, and an empty CPC is not expensive to
keep around.
PiperOrigin-RevId: 470256610
Change-Id: Ie14c6b0da19542947411998a9ac4ff1934f4cee4
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildSummaryStatsModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildSummaryStatsModule.java
index 0a6a75b..f051d65 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BuildSummaryStatsModule.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BuildSummaryStatsModule.java
@@ -88,6 +88,10 @@
public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) {
enabled = env.getOptions().getOptions(ExecutionOptions.class).enableCriticalPathProfiling;
statsSummary = env.getOptions().getOptions(ExecutionOptions.class).statsSummary;
+ if (enabled) {
+ criticalPathComputer = new CriticalPathComputer(actionKeyContext, BlazeClock.instance());
+ eventBus.register(criticalPathComputer);
+ }
}
@Subscribe
@@ -111,10 +115,6 @@
private void markExecutionPhaseStarted() {
// TODO(ulfjack): Make sure to use the same clock as for commandStartMillis.
executionStartMillis = BlazeClock.instance().currentTimeMillis();
- if (enabled) {
- criticalPathComputer = new CriticalPathComputer(actionKeyContext, BlazeClock.instance());
- eventBus.register(criticalPathComputer);
- }
}
@Subscribe
