Roll forward of https://github.com/bazelbuild/bazel/commit/656a0bab1e025ff3c27d595284a4bf1c5a8d8028 with test (unknown commit) and fix.

Big round of sandbox fixes / performance improvements.

- The number of stat() syscalls in the SymlinkedSandboxedSpawn was way too high. Do less, feel better.

- When using --experimental_sandbox_base, ensure that symlinks in the path are resolved. Before this, you had to check whether on your system /dev/shm is a symlink to /run/shm and then use that instead. Now it no longer matters, as symlinks are resolved.

- Remove an unnecessary directory creation from each sandboxed invocation. Turns out that the "tmpdir" that we created was no longer used after some changes to Bazel's TMPDIR handling.

- Use simpler sandbox paths, by using the unique ID for each Spawn provided by SpawnExecutionPolicy instead of a randomly generated temp folder name. This also saves a round-trip from our VFS to NIO and back. Clean up the sandbox base before each build to ensure that the unique IDs are actually unique. ;)

- Use Java 8's Process#isAlive to check whether a process is alive instead of trying to get the exitcode and catching an exception.

Closes #4913.

PiperOrigin-RevId: 193031017

6 files changed, 151 insertions, 76 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java
index fedd037d1c..ce05eee40e 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java
@@ -14,17 +14,17 @@
 
 package com.google.devtools.build.lib.worker;
 
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
 
 /** A {@link Worker} that runs inside a sandboxed execution root. */
 final class SandboxedWorker extends Worker {
   private final Path workDir;
+  private WorkerExecRoot workerExecRoot;
 
   SandboxedWorker(WorkerKey workerKey, int workerId, Path workDir, Path logFile) {
     super(workerKey, workerId, workDir, logFile);
@@ -38,35 +38,25 @@ final class SandboxedWorker extends Worker {
   }
 
   @Override
-  public void prepareExecution(WorkerKey key) throws IOException {
-    // Note: the key passed in here may be different from the key passed to the constructor for
-    // subsequent invocations of the same worker.
-    // TODO(ulfjack): Remove WorkerKey.getInputFiles and WorkerKey.getOutputFiles; they are only
-    // used to pass information to this method and the method below. Instead, don't pass the
-    // WorkerKey to this method but only the input and output files.
-    new SymlinkedSandboxedSpawn(
-            workDir,
-            workDir,
-            ImmutableList.of("/does_not_exist"),
-            ImmutableMap.of(),
-            key.getInputFiles(),
-            key.getOutputFiles(),
-            ImmutableSet.of())
-        .createFileSystem();
+  public void prepareExecution(
+      Map<PathFragment, Path> inputFiles,
+      Set<PathFragment> outputFiles,
+      Set<PathFragment> workerFiles)
+      throws IOException {
+    // Note that workerExecRoot isn't necessarily null at this point, so we can't do a Preconditions
+    // check for it: If a WorkerSpawnStrategy gets interrupted, finishExecution is not guaranteed to
+    // be called.
+    workerExecRoot = new WorkerExecRoot(workDir, inputFiles, outputFiles, workerFiles);
+    workerExecRoot.createFileSystem();
+
+    super.prepareExecution(inputFiles, outputFiles, workerFiles);
   }
 
   @Override
-  public void finishExecution(WorkerKey key) throws IOException {
-    // Note: the key passed in here may be different from the key passed to the constructor for
-    // subsequent invocations of the same worker.
-    new SymlinkedSandboxedSpawn(
-            workDir,
-            workDir,
-            ImmutableList.of("/does_not_exist"),
-            ImmutableMap.of(),
-            key.getInputFiles(),
-            key.getOutputFiles(),
-            ImmutableSet.of())
-        .copyOutputs(key.getExecRoot());
+  public void finishExecution(Path execRoot) throws IOException {
+    super.finishExecution(execRoot);
+
+    workerExecRoot.copyOutputs(execRoot);
+    workerExecRoot = null;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/Worker.java b/src/main/java/com/google/devtools/build/lib/worker/Worker.java
index 55705f4485..1cbd077e81 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/Worker.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/Worker.java
@@ -24,6 +24,8 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 import java.util.SortedMap;
 
 /**
@@ -54,17 +56,15 @@ class Worker {
 
     final Worker self = this;
     this.shutdownHook =
-        new Thread() {
-          @Override
-          public void run() {
-            try {
-              self.shutdownHook = null;
-              self.destroy();
-            } catch (IOException e) {
-              // We can't do anything here.
-            }
-          }
-        };
+        new Thread(
+            () -> {
+              try {
+                self.shutdownHook = null;
+                self.destroy();
+              } catch (IOException e) {
+                // We can't do anything here.
+              }
+            });
     Runtime.getRuntime().addShutdownHook(shutdownHook);
   }
 
@@ -80,7 +80,6 @@ class Worker {
     processBuilder.setWorkingDirectory(workDir.getPathFile());
     processBuilder.setStderr(logFile.getPathFile());
     processBuilder.setEnv(workerKey.getEnv());
-
     this.process = processBuilder.start();
   }
 
@@ -138,12 +137,7 @@ class Worker {
   boolean isAlive() {
     // This is horrible, but Process.isAlive() is only available from Java 8 on and this is the
     // best we can do prior to that.
-    try {
-      process.exitValue();
-      return false;
-    } catch (IllegalThreadStateException e) {
-      return true;
-    }
+    return !process.finished();
   }
 
   InputStream getInputStream() {
@@ -154,9 +148,17 @@ class Worker {
     return process.getOutputStream();
   }
 
-  public void prepareExecution(WorkerKey key) throws IOException {}
+  public void prepareExecution(
+      Map<PathFragment, Path> inputFiles,
+      Set<PathFragment> outputFiles,
+      Set<PathFragment> workerFiles)
+      throws IOException {
+    if (process == null) {
+      createProcess();
+    }
+  }
 
-  public void finishExecution(WorkerKey key) throws IOException {}
+  public void finishExecution(Path execRoot) throws IOException {}
 
   public Path getLogFile() {
     return logFile;
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java
new file mode 100644
index 0000000000..4cf02ac5b7
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java
@@ -0,0 +1,99 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.worker;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn;
+import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Symlinks;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+
+/** Creates and manages the contents of a working directory of a persistent worker. */
+final class WorkerExecRoot extends SymlinkedSandboxedSpawn {
+  private final Path workDir;
+  private final Set<PathFragment> workerFiles;
+
+  public WorkerExecRoot(
+      Path workDir,
+      Map<PathFragment, Path> inputs,
+      Collection<PathFragment> outputs,
+      Set<PathFragment> workerFiles) {
+    super(
+        workDir,
+        workDir,
+        ImmutableList.of(),
+        ImmutableMap.of(),
+        inputs,
+        outputs,
+        ImmutableSet.of());
+    this.workDir = workDir;
+    this.workerFiles = workerFiles;
+  }
+
+  @Override
+  public void createFileSystem() throws IOException {
+    workDir.createDirectoryAndParents();
+    deleteExceptAllowedFiles(workDir, workerFiles);
+    super.createFileSystem();
+  }
+
+  private void deleteExceptAllowedFiles(Path root, Set<PathFragment> allowedFiles)
+      throws IOException {
+    for (Path p : root.getDirectoryEntries()) {
+      FileStatus stat = p.stat(Symlinks.NOFOLLOW);
+      if (!stat.isDirectory()) {
+        if (!allowedFiles.contains(p.relativeTo(workDir))) {
+          p.delete();
+        }
+      } else {
+        deleteExceptAllowedFiles(p, allowedFiles);
+        if (p.readdir(Symlinks.NOFOLLOW).isEmpty()) {
+          p.delete();
+        }
+      }
+    }
+  }
+
+  @Override
+  protected void createInputs(Map<PathFragment, Path> inputs) throws IOException {
+    // All input files are relative to the execroot.
+    for (Entry<PathFragment, Path> entry : inputs.entrySet()) {
+      Path key = workDir.getRelative(entry.getKey());
+      FileStatus keyStat = key.statNullable(Symlinks.NOFOLLOW);
+      if (keyStat != null) {
+        if (keyStat.isSymbolicLink()
+            && entry.getValue() != null
+            && key.readSymbolicLink().equals(entry.getValue().asFragment())) {
+          continue;
+        }
+        key.delete();
+      }
+      // A null value means that we're supposed to create an empty file as the input.
+      if (entry.getValue() != null) {
+        key.createSymbolicLink(entry.getValue());
+      } else {
+        FileSystemUtils.createEmptyFile(key);
+      }
+    }
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
index 6f75fcf29d..871a3a0c64 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
@@ -65,8 +65,6 @@ final class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker
     } else {
       worker = new Worker(key, workerId, key.getExecRoot(), logFile);
     }
-    worker.prepareExecution(key);
-    worker.createProcess();
     if (workerOptions.workerVerbose) {
       reporter.handle(
           Event.info(
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
index 4641b5b710..4211419531 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
@@ -22,7 +22,6 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.SortedMap;
 
 /**
@@ -41,11 +40,7 @@ final class WorkerKey {
    * methods.
    */
   private final HashCode workerFilesCombinedHash;
-
   private final SortedMap<PathFragment, HashCode> workerFilesWithHashes;
-
-  private final Map<PathFragment, Path> inputFiles;
-  private final Set<PathFragment> outputFiles;
   private final boolean mustBeSandboxed;
 
   WorkerKey(
@@ -55,8 +50,6 @@ final class WorkerKey {
       String mnemonic,
       HashCode workerFilesCombinedHash,
       SortedMap<PathFragment, HashCode> workerFilesWithHashes,
-      Map<PathFragment, Path> inputFiles,
-      Set<PathFragment> outputFiles,
       boolean mustBeSandboxed) {
     this.args = ImmutableList.copyOf(Preconditions.checkNotNull(args));
     this.env = ImmutableMap.copyOf(Preconditions.checkNotNull(env));
@@ -64,8 +57,6 @@ final class WorkerKey {
     this.mnemonic = Preconditions.checkNotNull(mnemonic);
     this.workerFilesCombinedHash = Preconditions.checkNotNull(workerFilesCombinedHash);
     this.workerFilesWithHashes = Preconditions.checkNotNull(workerFilesWithHashes);
-    this.inputFiles = Preconditions.checkNotNull(inputFiles);
-    this.outputFiles = Preconditions.checkNotNull(outputFiles);
     this.mustBeSandboxed = mustBeSandboxed;
   }
 
@@ -93,14 +84,6 @@ final class WorkerKey {
     return workerFilesWithHashes;
   }
 
-  public Map<PathFragment, Path> getInputFiles() {
-    return inputFiles;
-  }
-
-  public Set<PathFragment> getOutputFiles() {
-    return outputFiles;
-  }
-
   public boolean mustBeSandboxed() {
     return mustBeSandboxed;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
index 1be61e1f77..a615cf76cd 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
@@ -151,14 +151,12 @@ final class WorkerSpawnRunner implements SpawnRunner {
             spawn.getMnemonic(),
             workerFilesCombinedHash,
             workerFiles,
-            inputFiles,
-            outputFiles,
             policy.speculating());
 
     WorkRequest workRequest = createWorkRequest(spawn, policy, flagFiles, inputFileCache);
 
     long startTime = System.currentTimeMillis();
-    WorkResponse response = execInWorker(key, workRequest, policy);
+    WorkResponse response = execInWorker(key, workRequest, policy, inputFiles, outputFiles);
     Duration wallTime = Duration.ofMillis(System.currentTimeMillis() - startTime);
 
     FileOutErr outErr = policy.getFileOutErr();
@@ -257,7 +255,12 @@ final class WorkerSpawnRunner implements SpawnRunner {
     }
   }
 
-  private WorkResponse execInWorker(WorkerKey key, WorkRequest request, SpawnExecutionPolicy policy)
+  private WorkResponse execInWorker(
+      WorkerKey key,
+      WorkRequest request,
+      SpawnExecutionPolicy policy,
+      Map<PathFragment, Path> inputFiles,
+      Set<PathFragment> outputFiles)
       throws InterruptedException, ExecException {
     Worker worker = null;
     WorkResponse response;
@@ -275,7 +278,7 @@ final class WorkerSpawnRunner implements SpawnRunner {
       }
 
       try {
-        worker.prepareExecution(key);
+        worker.prepareExecution(inputFiles, outputFiles, key.getWorkerFilesWithHashes().keySet());
       } catch (IOException e) {
         throw new UserExecException(
             ErrorMessage.builder()
@@ -337,7 +340,7 @@ final class WorkerSpawnRunner implements SpawnRunner {
       }
 
       try {
-        worker.finishExecution(key);
+        worker.finishExecution(execRoot);
       } catch (IOException e) {
         throw new UserExecException(
             ErrorMessage.builder()