remote: make the dynamic spawn scheduler work. Fixes #8646

This change fixes the correctness issue of dynamic spawn scheduler when being used with remote execution. See #8646 for more details. There's a performance issue remaining: #8467
bazelbuild · Jun 17, 2019 · f642b98 · f642b98
1 parent 82ec619
commit f642b98
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 6 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -304,6 +304,10 @@ private SpawnResult downloadAndFinalizeSpawnResult(
       SpawnExecutionContext context,
       RemoteOutputsMode remoteOutputsMode)
       throws ExecException, IOException, InterruptedException {
+    // Ensure that when using dynamic spawn strategy that we are the only ones writing to the
+    // output files. See https://github.com/bazelbuild/bazel/issues/8646 for more details.
+    context.lockOutputFiles();
+
     boolean downloadOutputs =
         shouldDownloadAllSpawnOutputs(
             remoteOutputsMode,

diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -521,14 +521,15 @@ public void testNonHumanReadableServerLogsNotSaved() throws Exception {
                 .build());
 
     Spawn spawn = newSimpleSpawn();
-    SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
+    FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
 
     SpawnResult res = runner.exec(spawn, policy);
     assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT);
 
     verify(executor).executeRemotely(any(ExecuteRequest.class));
     verify(cache).download(eq(result), eq(execRoot), any(FileOutErr.class));
     verify(cache, never()).downloadFile(any(Path.class), any(Digest.class));
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -547,14 +548,15 @@ public void testServerLogsNotSavedForSuccessfulAction() throws Exception {
                 .build());
 
     Spawn spawn = newSimpleSpawn();
-    SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
+    FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
 
     SpawnResult res = runner.exec(spawn, policy);
     assertThat(res.status()).isEqualTo(Status.SUCCESS);
 
     verify(executor).executeRemotely(any(ExecuteRequest.class));
     verify(cache).download(eq(result), eq(execRoot), any(FileOutErr.class));
     verify(cache, never()).downloadFile(any(Path.class), any(Digest.class));
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -866,7 +868,7 @@ public void testDownloadMinimalOnCacheMiss() throws Exception {
     RemoteSpawnRunner runner = newSpawnRunner();
 
     Spawn spawn = newSimpleSpawn();
-    SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
+    FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
 
     // act
     SpawnResult result = runner.exec(spawn, policy);
@@ -877,6 +879,7 @@ public void testDownloadMinimalOnCacheMiss() throws Exception {
     verify(executor).executeRemotely(any());
     verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any());
     verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr));
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -893,7 +896,7 @@ public void testDownloadMinimalIoError() throws Exception {
     RemoteSpawnRunner runner = newSpawnRunner();
 
     Spawn spawn = newSimpleSpawn();
-    SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
+    FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
 
     // act
     SpawnExecException e = assertThrows(SpawnExecException.class, () -> runner.exec(spawn, policy));
@@ -902,6 +905,7 @@ public void testDownloadMinimalIoError() throws Exception {
     // assert
     verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any());
     verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr));
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -920,7 +924,7 @@ public void testDownloadTopLevel() throws Exception {
     RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput));
 
     Spawn spawn = newSimpleSpawn(topLevelOutput);
-    SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
+    FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn);
 
     // act
     SpawnResult result = runner.exec(spawn, policy);
@@ -931,6 +935,7 @@ public void testDownloadTopLevel() throws Exception {
     verify(cache).download(eq(succeededAction), any(Path.class), eq(outErr));
     verify(cache, never())
         .downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any());
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   private static Spawn newSimpleSpawn(Artifact... outputs) {
@@ -998,6 +1003,8 @@ private RemoteSpawnRunner newSpawnRunnerWithoutExecutor(Reporter reporter) {
   // TODO(buchgr): Extract a common class to be used for testing.
   class FakeSpawnExecutionContext implements SpawnExecutionContext {
 
+    private boolean lockOutputFilesCalled;
+
     private final ArtifactExpander artifactExpander = (artifact, output) -> output.add(artifact);
 
     private final Spawn spawn;
@@ -1018,7 +1025,7 @@ public void prefetchInputs() {
 
     @Override
     public void lockOutputFiles() {
-      throw new UnsupportedOperationException();
+      lockOutputFilesCalled = true;
     }
 
     @Override