diff options
3 files changed, 63 insertions, 15 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 3677d2cd4f..b1bdc8a9f0 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,6 +18,7 @@ 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; @@ -48,6 +49,7 @@ import java.util.EnumMap; import java.util.List; import java.util.Map; import java.util.concurrent.ThreadLocalRandom; +import java.util.function.LongSupplier; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nullable; @@ -131,21 +133,27 @@ public final class LocalSpawnRunner implements SpawnRunner { } } - private static Path createActionTemp(Path execRoot) throws IOException { - 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(ThreadLocalRandom.current().nextLong()); - Path result = execRoot.getRelative("tmp" + idStr); - if (!result.exists() && !result.createDirectory()) { - throw new IOException(String.format("Could not create temp directory '%s'", result)); + @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 result; + return tempDirPath; } private final class SubprocessHandler { @@ -255,7 +263,7 @@ public final class LocalSpawnRunner implements SpawnRunner { stepLog(INFO, "running locally"); setState(State.LOCAL_ACTION_RUNNING); - Path tmpDir = createActionTemp(execRoot); + Path tmpDir = createActionTemp(execRoot, () -> ThreadLocalRandom.current().nextLong()); try { Command cmd; OutputStream stdOut = ByteStreams.nullOutputStream(); diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index 21c6eb59d9..ba38e5698e 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD @@ -1210,6 +1210,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", "//src/main/java/com/google/devtools/common/options", + "//src/test/java/com/google/devtools/build/lib:testutil", "//third_party:guava", "//third_party:junit4", "//third_party:mockito", 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 f7aa1d9ff3..554b66c3f9 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,6 +16,7 @@ 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.mockito.Matchers.any; import static org.mockito.Matchers.argThat; @@ -60,6 +61,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.logging.Filter; import java.util.logging.LogRecord; import java.util.logging.Logger; @@ -571,4 +573,41 @@ public class LocalSpawnRunnerTest { "/bin/echo", "Hi!")); } + + @Test + public void testCreateActionTemp_exceptionIfUnableToCreateDir() throws IOException { + 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 { + 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++; + } + } } |