Use java.nio.file.Files.createTempDirectory() to create temporary directory in LocalSpawnRunner, to avoid a race condition.

Aside:

The old, real temporary directory paths looked like this:

TMPDIR=/.../tmp15e_5dd5a8e8347813f5

The new, real temporary directory paths now look like this:

TMPDIR=/.../local-spawn-runner.3217503035718074040

RELNOTES: None.
PiperOrigin-RevId: 178903361

3 files changed, 140 insertions, 105 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
index bd5f8ff33b..716482c37a 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -18,7 +18,6 @@ import static java.util.logging.Level.INFO;
 import static java.util.logging.Level.SEVERE;
 import static java.util.logging.Level.WARNING;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.io.ByteStreams;
@@ -50,8 +49,6 @@ import java.util.EnumMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.concurrent.ThreadLocalRandom;
-import java.util.function.LongSupplier;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nullable;
@@ -61,7 +58,7 @@ import javax.annotation.Nullable;
  * completion.
  */
 @ThreadSafe
-public final class LocalSpawnRunner implements SpawnRunner {
+public class LocalSpawnRunner implements SpawnRunner {
   private static final Joiner SPACE_JOINER = Joiner.on(' ');
   private static final String UNHANDLED_EXCEPTION_MSG = "Unhandled exception running a local spawn";
   private static final int LOCAL_EXEC_ERROR = -1;
@@ -83,7 +80,7 @@ public final class LocalSpawnRunner implements SpawnRunner {
   private final LocalEnvProvider localEnvProvider;
 
   // TODO(b/62588075): Move this logic to ProcessWrapperUtil?
-  private static Path getProcessWrapper(Path execRoot, OS localOs) {
+  protected static Path getProcessWrapper(Path execRoot, OS localOs) {
     return execRoot.getRelative("_bin/process-wrapper" + OsUtils.executableExtension(localOs));
   }
 
@@ -135,31 +132,12 @@ public final class LocalSpawnRunner implements SpawnRunner {
     }
   }
 
-  @VisibleForTesting
-  static Path createActionTemp(Path execRoot, LongSupplier randomLongGenerator) throws IOException {
-    Path tempDirPath;
-    do {
-      String idStr =
-          // Make the name unique among other executor threads.
-          Long.toHexString(Thread.currentThread().getId())
-              + "_"
-              // Make the name unique among other temp directories that this thread has ever
-              // created.
-              // On Windows, file and directory deletion is asynchronous, meaning the previous temp
-              // directory name isn't immediately available for the next action that this thread
-              // runs.
-              // See https://github.com/bazelbuild/bazel/issues/4035
-              + Long.toHexString(randomLongGenerator.getAsLong());
-      tempDirPath = execRoot.getRelative("tmp" + idStr);
-    } while (tempDirPath.exists());
-    if (!tempDirPath.createDirectory()) {
-      throw new IOException(String.format("Could not create temp directory '%s'", tempDirPath));
-    }
-    return tempDirPath;
-  }
-
-  private static Path createActionTemp(Path execRoot) throws IOException {
-    return createActionTemp(execRoot, () -> ThreadLocalRandom.current().nextLong());
+  protected Path createActionTemp(Path execRoot) throws IOException {
+    return execRoot.getRelative(
+        java.nio.file.Files.createTempDirectory(
+                java.nio.file.Paths.get(execRoot.getPathString()), "local-spawn-runner.")
+            .getFileName()
+            .toString());
   }
 
   private final class SubprocessHandler {
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 c1d867e7c2..e99899d770 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
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.exec.local;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
-import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeTrue;
 import static org.mockito.Matchers.any;
@@ -69,7 +68,7 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.SortedMap;
 import java.util.TreeMap;
-import java.util.function.LongSupplier;
+import java.util.concurrent.ThreadLocalRandom;
 import java.util.logging.Filter;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
@@ -87,6 +86,45 @@ public class LocalSpawnRunnerTest {
   private static final boolean USE_WRAPPER = true;
   private static final boolean NO_WRAPPER = false;
 
+  private static class TestedLocalSpawnRunner extends LocalSpawnRunner {
+    public TestedLocalSpawnRunner(
+        Path execRoot,
+        LocalExecutionOptions localExecutionOptions,
+        ResourceManager resourceManager,
+        boolean useProcessWrapper,
+        OS localOs,
+        String productName,
+        LocalEnvProvider localEnvProvider) {
+      super(
+          execRoot,
+          localExecutionOptions,
+          resourceManager,
+          useProcessWrapper,
+          localOs,
+          productName,
+          localEnvProvider);
+    }
+
+    // Rigged to act on supplied filesystem (e.g. InMemoryFileSystem) for testing purposes
+    // TODO(b/70572634): Update FileSystem abstraction to support createTempDirectory() from
+    // the java.nio.file.Files package.
+    @Override
+    protected Path createActionTemp(Path execRoot) throws IOException {
+      Path tempDirPath;
+      do {
+        String idStr =
+            Long.toHexString(Thread.currentThread().getId())
+                + "_"
+                + Long.toHexString(ThreadLocalRandom.current().nextLong());
+        tempDirPath = execRoot.getRelative("tmp" + idStr);
+      } while (tempDirPath.exists());
+      if (!tempDirPath.createDirectory()) {
+        throw new IOException(String.format("Could not create temp directory '%s'", tempDirPath));
+      }
+      return tempDirPath;
+    }
+  }
+
   private static class FinishedSubprocess implements Subprocess {
     private final int exitCode;
 
@@ -222,7 +260,7 @@ public class LocalSpawnRunnerTest {
 
   @Before
   public final void suppressLogging() {
-    logger = Logger.getLogger(LocalSpawnRunner.class.getName());
+    logger = Logger.getLogger(TestedLocalSpawnRunner.class.getName());
     logger.setFilter(new Filter() {
       @Override
       public boolean isLoggable(LogRecord record) {
@@ -266,9 +304,15 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.localSigkillGraceSeconds = 456;
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnExecutionPolicyForTesting policy = new SpawnExecutionPolicyForTesting(fileOutErr);
@@ -315,9 +359,15 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.localSigkillGraceSeconds = 456;
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, NO_WRAPPER, OS.LINUX,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            NO_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnExecutionPolicyForTesting policy = new SpawnExecutionPolicyForTesting(fileOutErr);
@@ -354,9 +404,15 @@ public class LocalSpawnRunnerTest {
     SubprocessBuilder.setSubprocessFactory(factory);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -394,9 +450,15 @@ public class LocalSpawnRunnerTest {
     SubprocessBuilder.setSubprocessFactory(factory);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     assertThat(fs.getPath("/out").createDirectory()).isTrue();
     assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
@@ -424,9 +486,15 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.allowedLocalAction = Pattern.compile("none");
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
     FileOutErr fileOutErr = new FileOutErr();
@@ -469,9 +537,15 @@ public class LocalSpawnRunnerTest {
     SubprocessBuilder.setSubprocessFactory(factory);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnExecutionPolicyForTesting policy = new SpawnExecutionPolicyForTesting(fileOutErr);
@@ -495,9 +569,15 @@ public class LocalSpawnRunnerTest {
     SubprocessBuilder.setSubprocessFactory(factory);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnExecutionPolicyForTesting policy = new SpawnExecutionPolicyForTesting(fileOutErr);
@@ -516,9 +596,15 @@ public class LocalSpawnRunnerTest {
     SubprocessBuilder.setSubprocessFactory(factory);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnExecutionPolicyForTesting policy = new SpawnExecutionPolicyForTesting(fileOutErr);
@@ -541,9 +627,15 @@ public class LocalSpawnRunnerTest {
     LocalEnvProvider localEnvProvider = mock(LocalEnvProvider.class);
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.LINUX,
-        "product-name", localEnvProvider);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.LINUX,
+            "product-name",
+            localEnvProvider);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnExecutionPolicyForTesting policy = new SpawnExecutionPolicyForTesting(fileOutErr);
@@ -586,9 +678,15 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.localSigkillGraceSeconds = 654;
-    LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), options, resourceManager, USE_WRAPPER, OS.WINDOWS,
-        "product-name", LocalEnvProvider.UNMODIFIED);
+    LocalSpawnRunner runner =
+        new TestedLocalSpawnRunner(
+            fs.getPath("/execroot"),
+            options,
+            resourceManager,
+            USE_WRAPPER,
+            OS.WINDOWS,
+            "product-name",
+            LocalEnvProvider.UNMODIFIED);
 
     FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnExecutionPolicyForTesting policy = new SpawnExecutionPolicyForTesting(fileOutErr);
@@ -611,47 +709,6 @@ public class LocalSpawnRunnerTest {
                 "Hi!"));
   }
 
-  @Test
-  public void testCreateActionTemp_exceptionIfUnableToCreateDir() throws IOException {
-    FileSystem fs = setupEnvironmentForFakeExecution();
-
-    Path execRoot = fs.getPath("/execroot");
-    assertThat(execRoot.createDirectory()).isTrue();
-    assertThat(execRoot.exists()).isTrue();
-    execRoot.setWritable(false);
-
-    assertThrows(IOException.class, () -> LocalSpawnRunner.createActionTemp(execRoot, () -> 0));
-  }
-
-  @Test
-  public void testCreateActionTemp_retriesIfNameClashes() throws IOException {
-    FileSystem fs = setupEnvironmentForFakeExecution();
-
-    Path execRoot = fs.getPath("/execroot");
-    assertThat(execRoot.createDirectory()).isTrue();
-    assertThat(execRoot.exists()).isTrue();
-
-    Path tempPath1 = LocalSpawnRunner.createActionTemp(execRoot, () -> 0);
-    Path tempPath2 = LocalSpawnRunner.createActionTemp(execRoot, new IncreasingSequenceSupplier(0));
-
-    assertThat(tempPath1).isNotEqualTo(tempPath2);
-    assertThat(tempPath1.exists()).isTrue();
-    assertThat(tempPath2.exists()).isTrue();
-  }
-
-  private static class IncreasingSequenceSupplier implements LongSupplier {
-    private long currentElement;
-
-    public IncreasingSequenceSupplier(long startingElement) {
-      this.currentElement = startingElement;
-    }
-
-    @Override
-    public long getAsLong() {
-      return this.currentElement++;
-    }
-  }
-
   /**
    * Copies the {@code process-wrapper} tool into the path under the temporary execRoot where the
    * {@link LocalSpawnRunner} expects to find it.
diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh
index 2d8b2f387f..db897b1560 100755
--- a/src/test/shell/bazel/bazel_rules_test.sh
+++ b/src/test/shell/bazel/bazel_rules_test.sh
@@ -251,7 +251,7 @@ EOF
     || fail "Failed to build //pkg:test"
   assert_contains "PATH=$PATH_TO_BAZEL_WRAPPER:/bin:/usr/bin:/random/path" \
     bazel-genfiles/pkg/test.out
-  assert_contains "TMPDIR=.*execroot.*tmp[0-9a-fA-F]\+_[0-9a-fA-F]\+.*work$" \
+  assert_contains "TMPDIR=.*execroot.*local-spawn-runner.*work$" \
     bazel-genfiles/pkg/test.out
   assert_not_contains "TMPDIR=.*newfancytmpdir" \
     bazel-genfiles/pkg/test.out