Block when another command is running on the server and not on the client when in gRPC mode.

--
MOS_MIGRATED_REVID=120233121

8 files changed, 102 insertions, 24 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index eabb448529..707aabe259 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -17,6 +17,7 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.base.Predicates;
+import com.google.common.base.Verify;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -34,6 +35,7 @@ import com.google.devtools.build.lib.util.AnsiStrippingOutputStream;
 import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.util.LoggingUtil;
 import com.google.devtools.build.lib.util.Pair;
+import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.DelegatingOutErr;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -65,6 +67,13 @@ import javax.annotation.Nullable;
  */
 public class BlazeCommandDispatcher {
 
+  /**
+   * What to do if the command lock is not available.
+   */
+  public enum LockingMode {
+    WAIT,  // Wait until it is available
+    ERROR_OUT,  // Return with an error
+  }
   // Keep in sync with options added in OptionProcessor::AddRcfileArgsAndOptions()
   private static final Set<String> INTERNAL_COMMAND_OPTIONS = ImmutableSet.of(
       "rc_source", "default_override", "isatty", "terminal_columns", "ignore_client_env",
@@ -97,7 +106,8 @@ public class BlazeCommandDispatcher {
   }
 
   private final BlazeRuntime runtime;
-
+  private final Object commandLock;
+  private String currentClientDescription = null;
   private OutputStream logOutputStream = null;
 
   /**
@@ -116,6 +126,7 @@ public class BlazeCommandDispatcher {
   @VisibleForTesting
   public BlazeCommandDispatcher(BlazeRuntime runtime) {
     this.runtime = runtime;
+    this.commandLock = new Object();
   }
 
   /**
@@ -208,19 +219,13 @@ public class BlazeCommandDispatcher {
    * client process, or throws {@link ShutdownBlazeServerException} to
    * indicate that a command wants to shutdown the Blaze server.
    */
-  int exec(List<String> args, OutErr outErr, long firstContactTime)
-      throws ShutdownBlazeServerException {
-    // Record the start time for the profiler. Do not put anything before this!
-    long execStartTimeNanos = runtime.getClock().nanoTime();
-
-    // The initCommand call also records the start time for the timestamp granularity monitor.
-    CommandEnvironment env = runtime.getWorkspace().initCommand();
-    // Record the command's starting time for use by the commands themselves.
-    env.recordCommandStartTime(firstContactTime);
-
+  int exec(List<String> args, OutErr outErr, LockingMode lockingMode, String clientDescription,
+      long firstContactTime) throws ShutdownBlazeServerException, InterruptedException {
+    Preconditions.checkNotNull(clientDescription);
     if (args.isEmpty()) { // Default to help command if no arguments specified.
       args = HELP_COMMAND;
     }
+
     String commandName = args.get(0);
 
     // Be gentle to users who want to find out about Blaze invocation.
@@ -234,8 +239,52 @@ public class BlazeCommandDispatcher {
           "Command '%s' not found. Try '%s help'.", commandName, Constants.PRODUCT_NAME));
       return ExitCode.COMMAND_LINE_ERROR.getNumericExitCode();
     }
+
+
+    synchronized (commandLock) {
+      if (currentClientDescription != null) {
+        switch (lockingMode) {
+          case WAIT:
+            outErr.printErrLn("Another command (" + currentClientDescription + ") is running. "
+                + " Waiting for it to complete...");
+            commandLock.wait();
+            break;
+
+          case ERROR_OUT:
+            outErr.printErrLn(String.format("Another command (" + currentClientDescription + ") is "
+                + "running. Exiting immediately."));
+            return ExitCode.COMMAND_LINE_ERROR.getNumericExitCode();
+
+          default:
+            throw new IllegalStateException();
+        }
+      }
+      Verify.verify(currentClientDescription == null);
+      currentClientDescription = clientDescription;
+    }
+
+    try {
+      return execExclusively(args, outErr, firstContactTime, commandName, command);
+    } finally {
+      synchronized (commandLock) {
+        currentClientDescription = null;
+        commandLock.notify();
+      }
+    }
+  }
+
+  private int execExclusively(List<String> args, OutErr outErr, long firstContactTime,
+      String commandName, BlazeCommand command) throws ShutdownBlazeServerException {
     Command commandAnnotation = command.getClass().getAnnotation(Command.class);
 
+    // Record the start time for the profiler. Do not put anything before this!
+    long execStartTimeNanos = runtime.getClock().nanoTime();
+
+    // The initCommand call also records the start time for the timestamp granularity monitor.
+    CommandEnvironment env = runtime.getWorkspace().initCommand();
+    // Record the command's starting time for use by the commands themselves.
+    env.recordCommandStartTime(firstContactTime);
+
     AbruptExitException exitCausingException = null;
     for (BlazeModule module : runtime.getBlazeModules()) {
       try {
@@ -410,12 +459,14 @@ public class BlazeCommandDispatcher {
   }
 
   /**
-   * For testing ONLY. Same as {@link #exec(List, OutErr, long)}, but automatically uses the current
-   * time.
+   * For testing ONLY. Same as {@link #exec(List, OutErr, boolean, String, long)}, but automatically
+   * uses the current time.
    */
   @VisibleForTesting
-  public int exec(List<String> args, OutErr originalOutErr) throws ShutdownBlazeServerException {
-    return exec(args, originalOutErr, runtime.getClock().currentTimeMillis());
+  public int exec(List<String> args, LockingMode lockingMode, String clientDescription,
+      OutErr originalOutErr) throws ShutdownBlazeServerException, InterruptedException {
+    return exec(args, originalOutErr, LockingMode.ERROR_OUT, clientDescription,
+        runtime.getClock().currentTimeMillis());
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
index 9d612693d4..2004ffe64a 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.query2.AbstractBlazeQueryEnvironment;
 import com.google.devtools.build.lib.query2.QueryEnvironmentFactory;
 import com.google.devtools.build.lib.query2.output.OutputFormatter;
 import com.google.devtools.build.lib.rules.test.CoverageReportActionFactory;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.LockingMode;
 import com.google.devtools.build.lib.runtime.commands.BuildCommand;
 import com.google.devtools.build.lib.runtime.commands.CanonicalizeCommand;
 import com.google.devtools.build.lib.runtime.commands.CleanCommand;
@@ -820,9 +821,12 @@ public final class BlazeRuntime {
     try {
       LOG.info(getRequestLogString(commandLineOptions.getOtherArgs()));
       return dispatcher.exec(commandLineOptions.getOtherArgs(), OutErr.SYSTEM_OUT_ERR,
-          runtime.getClock().currentTimeMillis());
+          LockingMode.ERROR_OUT, "batch client", runtime.getClock().currentTimeMillis());
     } catch (BlazeCommandDispatcher.ShutdownBlazeServerException e) {
       return e.getExitStatus();
+    } catch (InterruptedException e) {
+      // This is almost main(), so it's okay to just swallow it. We are exiting soon.
+      return ExitCode.INTERRUPTED.getNumericExitCode();
     } finally {
       runtime.shutdown();
       dispatcher.shutdown();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/Command.java b/src/main/java/com/google/devtools/build/lib/runtime/Command.java
index 83819eb9a4..c415f0a4cf 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/Command.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/Command.java
@@ -117,5 +117,4 @@ public @interface Command {
    * accept several argument types, they can be combined with |, e.g <code>label|path</code>.
    */
   String completion() default "";
-
 }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
index 659f4b7136..f1560b4ba5 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
@@ -40,11 +40,12 @@ public class CommandExecutor implements ServerCommand {
   }
 
   @Override
-  public int exec(List<String> args, OutErr outErr, long firstContactTime) {
+  public int exec(List<String> args, OutErr outErr, BlazeCommandDispatcher.LockingMode lockingMode,
+      String clientDescription, long firstContactTime) throws InterruptedException {
     LOG.info(BlazeRuntime.getRequestLogString(args));
 
     try {
-      return dispatcher.exec(args, outErr, firstContactTime);
+      return dispatcher.exec(args, outErr, lockingMode, clientDescription, firstContactTime);
     } catch (BlazeCommandDispatcher.ShutdownBlazeServerException e) {
       if (e.getCause() != null) {
         StringWriter message = new StringWriter();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
index c23cbf2d72..68df6a7258 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
@@ -247,4 +247,12 @@ public class CommonCommandOptions extends OptionsBase {
       category = "hidden",
       help = "Enable processing of +<file> parameters.")
   public boolean allowProjectFiles;
+
+  @Option(name = "block_for_lock",
+      defaultValue = "true",
+      category = "hidden",
+      help = "If set (the default), a command will block if there is another one running. If "
+          + "unset, these commands will immediately return with an error.")
+  public boolean blockForLock;
+
 }
diff --git a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
index 58f0487873..8509a25389 100644
--- a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.server;
 
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.LockingMode;
 import com.google.devtools.build.lib.runtime.CommandExecutor;
 import com.google.devtools.build.lib.server.CommandProtos.CancelRequest;
 import com.google.devtools.build.lib.server.CommandProtos.CancelResponse;
@@ -235,7 +236,8 @@ public class GrpcServerImpl extends RPCServer implements CommandServerGrpc.Comma
   @Override
   public void run(
       RunRequest request, StreamObserver<RunResponse> observer) {
-    if (!request.getCookie().equals(requestCookie)) {
+    if (!request.getCookie().equals(requestCookie)
+        || request.getClientDescription().isEmpty()) {
       observer.onNext(RunResponse.newBuilder()
           .setExitCode(ExitCode.LOCAL_ENVIRONMENTAL_ERROR.getNumericExitCode())
           .build());
@@ -251,13 +253,19 @@ public class GrpcServerImpl extends RPCServer implements CommandServerGrpc.Comma
           new RpcOutputStream(observer, command.id, StreamType.STDOUT),
           new RpcOutputStream(observer, command.id, StreamType.STDERR));
 
-      exitCode = commandExecutor.exec(request.getArgList(), rpcOutErr, clock.currentTimeMillis());
+      exitCode = commandExecutor.exec(
+          request.getArgList(), rpcOutErr,
+          request.getBlockForLock() ? LockingMode.WAIT : LockingMode.ERROR_OUT,
+          request.getClientDescription(), clock.currentTimeMillis());
+    } catch (InterruptedException e) {
+      exitCode = ExitCode.INTERRUPTED.getNumericExitCode();
+      commandId = "";  // The default value, the client will ignore it
     }
 
     // There is a chance that a cancel request comes in after commandExecutor#exec() has finished
     // and no one calls Thread.interrupted() to receive the interrupt. So we just reset the
     // interruption state here to make these cancel requests not have any effect outside of command
-    // execution.
+    // execution (after the try block above, the cancel request won't find the thread to interrupt)
     Thread.interrupted();
 
     RunResponse response = RunResponse.newBuilder()
diff --git a/src/main/java/com/google/devtools/build/lib/server/RPCService.java b/src/main/java/com/google/devtools/build/lib/server/RPCService.java
index ebaa8b76ad..ab40553b31 100644
--- a/src/main/java/com/google/devtools/build/lib/server/RPCService.java
+++ b/src/main/java/com/google/devtools/build/lib/server/RPCService.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.server;
 
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.LockingMode;
 import com.google.devtools.build.lib.util.io.OutErr;
 
 import java.util.List;
@@ -62,7 +63,11 @@ public final class RPCService {
     }
     String command = Iterables.getFirst(request, "");
     if (appCommand != null && command.equals("blaze")) { // an application request
-      int result = appCommand.exec(request.subList(1, request.size()), outErr, firstContactTime);
+      // Blocking is done in the client for AF_UNIX communications, so if blockForLock would block,
+      // something went wrong
+      int result = appCommand.exec(
+          request.subList(1, request.size()), outErr, LockingMode.ERROR_OUT, "AF_UNIX client",
+          firstContactTime);
       if (appCommand.shutdown()) { // an application shutdown request
         shutdown();
       }
diff --git a/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java b/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
index f10689b319..577cb4871f 100644
--- a/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.server;
 
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher;
 import com.google.devtools.build.lib.util.io.OutErr;
 
 import java.util.List;
@@ -27,7 +28,8 @@ public interface ServerCommand {
    * Executes the request, writing any output or error messages into err.
    * Returns 0 on success; any other value or exception indicates an error.
    */
-  int exec(List<String> args, OutErr outErr, long firstContactTime) throws Exception;
+  int exec(List<String> args, OutErr outErr, BlazeCommandDispatcher.LockingMode lockingMode,
+      String clientDescription, long firstContactTime) throws InterruptedException;
 
   /**
    * The implementation returns true from this method to initiate a shutdown.