Internal change

PiperOrigin-RevId: 191642942

7 files changed, 79 insertions, 152 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 a25ce97c0c..fedd037d1c 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,18 +14,17 @@
 
 package com.google.devtools.build.lib.worker;
 
-import com.google.common.base.Preconditions;
+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);
@@ -39,23 +38,35 @@ final class SandboxedWorker extends Worker {
   }
 
   @Override
-  public void prepareExecution(
-      Map<PathFragment, Path> inputFiles,
-      Set<PathFragment> outputFiles,
-      Set<PathFragment> workerFiles)
-      throws IOException {
-    Preconditions.checkState(workerExecRoot == null);
-    this.workerExecRoot = new WorkerExecRoot(workDir, inputFiles, outputFiles, workerFiles);
-    workerExecRoot.createFileSystem();
-
-    super.prepareExecution(inputFiles, outputFiles, workerFiles);
+  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();
   }
 
   @Override
-  public void finishExecution(Path execRoot) throws IOException {
-    super.finishExecution(execRoot);
-
-    workerExecRoot.copyOutputs(execRoot);
-    workerExecRoot = null;
+  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());
   }
 }
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 1cbd077e81..55705f4485 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,8 +24,6 @@ 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;
 
 /**
@@ -56,15 +54,17 @@ class Worker {
 
     final Worker self = this;
     this.shutdownHook =
-        new Thread(
-            () -> {
-              try {
-                self.shutdownHook = null;
-                self.destroy();
-              } catch (IOException e) {
-                // We can't do anything here.
-              }
-            });
+        new Thread() {
+          @Override
+          public void run() {
+            try {
+              self.shutdownHook = null;
+              self.destroy();
+            } catch (IOException e) {
+              // We can't do anything here.
+            }
+          }
+        };
     Runtime.getRuntime().addShutdownHook(shutdownHook);
   }
 
@@ -80,6 +80,7 @@ class Worker {
     processBuilder.setWorkingDirectory(workDir.getPathFile());
     processBuilder.setStderr(logFile.getPathFile());
     processBuilder.setEnv(workerKey.getEnv());
+
     this.process = processBuilder.start();
   }
 
@@ -137,7 +138,12 @@ 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.
-    return !process.finished();
+    try {
+      process.exitValue();
+      return false;
+    } catch (IllegalThreadStateException e) {
+      return true;
+    }
   }
 
   InputStream getInputStream() {
@@ -148,17 +154,9 @@ class Worker {
     return process.getOutputStream();
   }
 
-  public void prepareExecution(
-      Map<PathFragment, Path> inputFiles,
-      Set<PathFragment> outputFiles,
-      Set<PathFragment> workerFiles)
-      throws IOException {
-    if (process == null) {
-      createProcess();
-    }
-  }
+  public void prepareExecution(WorkerKey key) throws IOException {}
 
-  public void finishExecution(Path execRoot) throws IOException {}
+  public void finishExecution(WorkerKey key) 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
deleted file mode 100644
index 4cf02ac5b7..0000000000
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java
+++ /dev/null
@@ -1,99 +0,0 @@
-// 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 871a3a0c64..6f75fcf29d 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,6 +65,8 @@ 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 4211419531..4641b5b710 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,6 +22,7 @@ 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;
 
 /**
@@ -40,7 +41,11 @@ 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(
@@ -50,6 +55,8 @@ 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));
@@ -57,6 +64,8 @@ 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;
   }
 
@@ -84,6 +93,14 @@ 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 a615cf76cd..1be61e1f77 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,12 +151,14 @@ 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, inputFiles, outputFiles);
+    WorkResponse response = execInWorker(key, workRequest, policy);
     Duration wallTime = Duration.ofMillis(System.currentTimeMillis() - startTime);
 
     FileOutErr outErr = policy.getFileOutErr();
@@ -255,12 +257,7 @@ final class WorkerSpawnRunner implements SpawnRunner {
     }
   }
 
-  private WorkResponse execInWorker(
-      WorkerKey key,
-      WorkRequest request,
-      SpawnExecutionPolicy policy,
-      Map<PathFragment, Path> inputFiles,
-      Set<PathFragment> outputFiles)
+  private WorkResponse execInWorker(WorkerKey key, WorkRequest request, SpawnExecutionPolicy policy)
       throws InterruptedException, ExecException {
     Worker worker = null;
     WorkResponse response;
@@ -278,7 +275,7 @@ final class WorkerSpawnRunner implements SpawnRunner {
       }
 
       try {
-        worker.prepareExecution(inputFiles, outputFiles, key.getWorkerFilesWithHashes().keySet());
+        worker.prepareExecution(key);
       } catch (IOException e) {
         throw new UserExecException(
             ErrorMessage.builder()
@@ -340,7 +337,7 @@ final class WorkerSpawnRunner implements SpawnRunner {
       }
 
       try {
-        worker.finishExecution(execRoot);
+        worker.finishExecution(key);
       } catch (IOException e) {
         throw new UserExecException(
             ErrorMessage.builder()
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
index 013dbe226a..2302fc7c1e 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
@@ -144,9 +144,10 @@ public class WorkerTestStrategy extends StandaloneTestStrategy {
               action.getMnemonic(),
               workerFilesCombinedHash,
               workerFiles,
+              ImmutableMap.<PathFragment, Path>of(),
+              ImmutableSet.<PathFragment>of(),
               /*mustBeSandboxed=*/ false);
       worker = workerPool.borrowObject(key);
-      worker.prepareExecution(ImmutableMap.of(), ImmutableSet.of(), workerFiles.keySet());
 
       WorkRequest request = WorkRequest.getDefaultInstance();
       request.writeDelimitedTo(worker.getOutputStream());
@@ -175,7 +176,7 @@ public class WorkerTestStrategy extends StandaloneTestStrategy {
         throw e;
       }
 
-      worker.finishExecution(execRoot);
+      worker.finishExecution(key);
 
       if (response == null) {
         throw new UserExecException(