Extend the SpawnRunner API

- add an id for logging; this allows us to correlate log entries for the same
  spawn from multiple spawn runner implementations in the future
- add a prefetch method to the SpawnExecutionPolicy; better than relying on
  the ActionInputPrefetcher being injected in the constructor
- add a name parameter to the report method; this is in preparation for a
  single unified SpawnStrategy implementation - it's basically the last bit of
  difference between SandboxStrategy and RemoteSpawnStrategy; they're otherwise
  equivalent (if not identical)

PiperOrigin-RevId: 162194684

4 files changed, 93 insertions, 59 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
index 6ee2b0f11f..13eea28d36 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -18,11 +18,11 @@ import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
-import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.Spawn;
-import com.google.devtools.build.lib.exec.ActionInputPrefetcher;
 import com.google.devtools.build.lib.exec.SpawnResult;
 import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
@@ -58,7 +57,6 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.SortedMap;
 import java.util.TreeMap;
-import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Filter;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
@@ -145,9 +143,23 @@ public class LocalSpawnRunnerTest {
     private final List<ProgressStatus> reportedStatus = new ArrayList<>();
     private final TreeMap<PathFragment, ActionInput> inputMapping = new TreeMap<>();
 
+    private long timeoutMillis;
+    private final List<Iterable<ActionInput>> prefetched = new ArrayList<>();
+    private boolean lockOutputFilesCalled;
+
+    @Override
+    public int getId() {
+      return 0;
+    }
+
+    @Override
+    public void prefetchInputs(Iterable<ActionInput> inputs) throws IOException {
+      prefetched.add(Preconditions.checkNotNull(inputs));
+    }
+
     @Override
     public void lockOutputFiles() throws InterruptedException {
-      calledLockOutputFiles = true;
+      lockOutputFilesCalled = true;
     }
 
     @Override
@@ -176,7 +188,7 @@ public class LocalSpawnRunnerTest {
     }
 
     @Override
-    public void report(ProgressStatus state) {
+    public void report(ProgressStatus state, String name) {
       reportedStatus.add(state);
     }
   }
@@ -186,10 +198,7 @@ public class LocalSpawnRunnerTest {
   private final ResourceManager resourceManager = ResourceManager.instanceForTestingOnly();
 
   private Logger logger;
-  private final AtomicInteger execCount = new AtomicInteger();
   private FileOutErr outErr;
-  private long timeoutMillis = 0;
-  private boolean calledLockOutputFiles;
 
   private final SpawnExecutionPolicyForTesting policy = new SpawnExecutionPolicyForTesting();
 
@@ -228,10 +237,10 @@ public class LocalSpawnRunnerTest {
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.localSigkillGraceSeconds = 456;
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options,
-        resourceManager, USE_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
-    timeoutMillis = 123 * 1000L;
+    policy.timeoutMillis = 123 * 1000L;
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnResult result = runner.exec(SIMPLE_SPAWN, policy);
     verify(factory).create(any(SubprocessBuilder.class));
@@ -253,7 +262,7 @@ public class LocalSpawnRunnerTest {
     assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
     assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(-1);
 
-    assertThat(calledLockOutputFiles).isTrue();
+    assertThat(policy.lockOutputFilesCalled).isTrue();
     assertThat(policy.reportedStatus)
         .containsExactly(ProgressStatus.SCHEDULING, ProgressStatus.EXECUTING).inOrder();
   }
@@ -268,10 +277,10 @@ public class LocalSpawnRunnerTest {
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.localSigkillGraceSeconds = 456;
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options,
-        resourceManager, NO_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, NO_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
-    timeoutMillis = 123 * 1000L;
+    policy.timeoutMillis = 123 * 1000L;
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnResult result = runner.exec(SIMPLE_SPAWN, policy);
     verify(factory).create(any());
@@ -284,9 +293,9 @@ public class LocalSpawnRunnerTest {
         .containsExactlyElementsIn(ImmutableList.of("/bin/echo", "Hi!"));
     assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
     // Without the process wrapper, we use the Command API to enforce the timeout.
-    assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(timeoutMillis);
+    assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(policy.timeoutMillis);
 
-    assertThat(calledLockOutputFiles).isTrue();
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -298,8 +307,8 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options,
-        resourceManager, USE_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnResult result = runner.exec(SIMPLE_SPAWN, policy);
@@ -322,7 +331,7 @@ public class LocalSpawnRunnerTest {
                 "Hi!"));
     assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
 
-    assertThat(calledLockOutputFiles).isTrue();
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -334,8 +343,8 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options,
-        resourceManager, USE_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
     assertThat(fs.getPath("/out").createDirectory()).isTrue();
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -350,7 +359,7 @@ public class LocalSpawnRunnerTest {
     assertThat(FileSystemUtils.readContent(fs.getPath("/out/stderr"), StandardCharsets.UTF_8))
         .isEqualTo("Action failed to execute: java.io.IOException: I'm sorry, Dave\n");
 
-    assertThat(calledLockOutputFiles).isTrue();
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -358,8 +367,8 @@ public class LocalSpawnRunnerTest {
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.allowedLocalAction = Pattern.compile("none");
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options,
-        resourceManager, USE_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
     outErr = new FileOutErr();
     SpawnResult reply = runner.exec(SIMPLE_SPAWN, policy);
@@ -370,7 +379,7 @@ public class LocalSpawnRunnerTest {
     assertThat(reply.getExecutorHostName()).isEqualTo(NetUtil.findShortHostName());
 
     // TODO(ulfjack): Maybe we should only lock after checking?
-    assertThat(calledLockOutputFiles).isTrue();
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -397,8 +406,8 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options,
-        resourceManager, USE_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     try {
@@ -408,7 +417,7 @@ public class LocalSpawnRunnerTest {
       // Clear the interrupted status or subsequent tests in the same process will fail.
       Thread.interrupted();
     }
-    assertThat(calledLockOutputFiles).isTrue();
+    assertThat(policy.lockOutputFilesCalled).isTrue();
   }
 
   @Test
@@ -416,17 +425,16 @@ public class LocalSpawnRunnerTest {
     Subprocess.Factory factory = mock(Subprocess.Factory.class);
     when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
     SubprocessBuilder.setSubprocessFactory(factory);
-    ActionInputPrefetcher mockPrefetcher = mock(ActionInputPrefetcher.class);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), mockPrefetcher, options, resourceManager,
-        USE_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
-    timeoutMillis = 123 * 1000L;
+    policy.timeoutMillis = 123 * 1000L;
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     runner.exec(SIMPLE_SPAWN, policy);
-    verify(mockPrefetcher).prefetchFiles(any());
+    assertThat(policy.prefetched).isNotEmpty();
   }
 
   @Test
@@ -434,51 +442,46 @@ public class LocalSpawnRunnerTest {
     Subprocess.Factory factory = mock(Subprocess.Factory.class);
     when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
     SubprocessBuilder.setSubprocessFactory(factory);
-    ActionInputPrefetcher mockPrefetcher = mock(ActionInputPrefetcher.class);
-    doThrow(new RuntimeException("Called prefetch!")).when(mockPrefetcher).prefetchFiles(any());
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), mockPrefetcher, options, resourceManager,
-        USE_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
-    timeoutMillis = 123 * 1000L;
+    policy.timeoutMillis = 123 * 1000L;
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
 
     Spawn spawn = new SpawnBuilder("/bin/echo", "Hi!")
         .withExecutionInfo(ExecutionRequirements.DISABLE_LOCAL_PREFETCH, "").build();
-    // This would throw if the runner called prefetchFiles().
     runner.exec(spawn, policy);
+    assertThat(policy.prefetched).isEmpty();
   }
 
   /**
    * Regression test: the SpawnInputExpander can return null values for empty files, but the
    * ActionInputPrefetcher expects no null values.
    */
-  @SuppressWarnings("unchecked")
   @Test
   public void checkPrefetchCalledNonNull() throws Exception {
     Subprocess.Factory factory = mock(Subprocess.Factory.class);
     when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
     SubprocessBuilder.setSubprocessFactory(factory);
-    ActionInputPrefetcher mockPrefetcher = mock(ActionInputPrefetcher.class);
-    @SuppressWarnings("rawtypes")
-    ArgumentCaptor<Iterable> captor = ArgumentCaptor.forClass(Iterable.class);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), mockPrefetcher, options, resourceManager,
-        USE_WRAPPER, OS.LINUX, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
     policy.inputMapping.put(PathFragment.create("relative/path"), null);
     policy.inputMapping.put(
         PathFragment.create("another/relative/path"), ActionInputHelper.fromPath("/absolute/path"));
-    timeoutMillis = 123 * 1000L;
+    policy.timeoutMillis = 123 * 1000L;
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     runner.exec(SIMPLE_SPAWN, policy);
-    verify(mockPrefetcher).prefetchFiles(captor.capture());
-    assertThat(captor.getValue()).doesNotContain(null);
-    assertThat(captor.getValue()).containsExactly(ActionInputHelper.fromPath("/absolute/path"));
+    assertThat(policy.prefetched).hasSize(1);
+    Iterable<ActionInput> prefetched = policy.prefetched.get(0);
+    assertThat(prefetched).doesNotContain(null);
+    assertThat(prefetched).containsExactly(ActionInputHelper.fromPath("/absolute/path"));
   }
 
   @Test
@@ -490,10 +493,10 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options,
-        resourceManager, USE_WRAPPER, OS.LINUX, "product-name", localEnvProvider);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
+        "product-name", localEnvProvider);
 
-    timeoutMillis = 123 * 1000L;
+    policy.timeoutMillis = 123 * 1000L;
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
 
     runner.exec(SIMPLE_SPAWN, policy);
@@ -511,10 +514,10 @@ public class LocalSpawnRunnerTest {
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.localSigkillGraceSeconds = 654;
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        execCount, fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options,
-        resourceManager, USE_WRAPPER, OS.WINDOWS, "product-name", LocalEnvProvider.UNMODIFIED);
+        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.WINDOWS,
+        "product-name", LocalEnvProvider.UNMODIFIED);
 
-    timeoutMillis = 321 * 1000L;
+    policy.timeoutMillis = 321 * 1000L;
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnResult result = runner.exec(SIMPLE_SPAWN, policy);
     verify(factory).create(any(SubprocessBuilder.class));
diff --git a/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java
index 882e304a30..d6cd69c417 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/CachedLocalSpawnRunnerTest.java
@@ -81,6 +81,17 @@ public class CachedLocalSpawnRunnerTest {
   private final SpawnExecutionPolicy simplePolicy =
       new SpawnExecutionPolicy() {
         @Override
+        public int getId() {
+          return 0;
+        }
+
+        @Override
+        public void prefetchInputs(Iterable<ActionInput> inputs) {
+          // CachedLocalSpawnRunner should never prefetch itself, though the nested SpawnRunner may.
+          throw new UnsupportedOperationException();
+        }
+
+        @Override
         public void lockOutputFiles() throws InterruptedException {
           throw new UnsupportedOperationException();
         }
@@ -112,7 +123,7 @@ public class CachedLocalSpawnRunnerTest {
         }
 
         @Override
-        public void report(ProgressStatus state) {
+        public void report(ProgressStatus state, String name) {
           // TODO(ulfjack): Test that the right calls are made.
         }
       };
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
index a773d4d1a1..6003a70577 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
@@ -108,6 +108,16 @@ public class GrpcRemoteExecutionClientTest {
   private final SpawnExecutionPolicy simplePolicy =
       new SpawnExecutionPolicy() {
         @Override
+        public int getId() {
+          return 0;
+        }
+
+        @Override
+        public void prefetchInputs(Iterable<ActionInput> inputs) {
+          throw new UnsupportedOperationException();
+        }
+
+        @Override
         public void lockOutputFiles() throws InterruptedException {
           throw new UnsupportedOperationException();
         }
@@ -139,7 +149,7 @@ public class GrpcRemoteExecutionClientTest {
         }
 
         @Override
-        public void report(ProgressStatus state) {
+        public void report(ProgressStatus state, String name) {
           // TODO(ulfjack): Test that the right calls are made.
         }
       };
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
index 0390994284..a6d0be8f13 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -186,6 +186,16 @@ public class RemoteSpawnRunnerTest {
     }
 
     @Override
+    public int getId() {
+      return 0;
+    }
+
+    @Override
+    public void prefetchInputs(Iterable<ActionInput> inputs) throws IOException {
+      throw new UnsupportedOperationException();
+    }
+
+    @Override
     public void lockOutputFiles() throws InterruptedException {
       throw new UnsupportedOperationException();
     }
@@ -217,7 +227,7 @@ public class RemoteSpawnRunnerTest {
     }
 
     @Override
-    public void report(ProgressStatus state) {
+    public void report(ProgressStatus state, String name) {
       assertThat(state).isEqualTo(ProgressStatus.EXECUTING);
     }
   }