Fix a race condition introduced in unknown commit (that is, the January of 2009!).

If a "blaze clean --expunge" was run concurrently with another command (that was waiting for the lock), it's possible that the clean command deletes the lock file, the new server starts up, then the JVM shutdown hooks delete the PID files from the *new* server.

There is still a slight possibility of a race condition if the lock is deleted then IOException occurs which prevents the BlazeShutdownException from being raised, but I'd rather not introduce another channel from command implementations to RPCServer to close that loophole.

This issue was triggered by commit 5a78166ee4edbd295f5d5fdb94785025285e764b, after which the PID files for the new server are written a bit more early, thus increasing the time window in which the race condition can happen.

--
MOS_MIGRATED_REVID=120805163

9 files changed, 104 insertions, 36 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 5f288d7487..5abddfe86b 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
@@ -84,25 +84,62 @@ public class BlazeCommandDispatcher {
   private static final Set<String> ALL_HELP_OPTIONS = ImmutableSet.of("--help", "-help", "-h");
 
   /**
+   * If the server needs to be shut down and how.
+   */
+  public enum ShutdownMethod {
+    /** The server doesn't need to be shut down. */
+    NONE,
+
+    /** The server is to be shut down cleanly, e.g. as a result of "blaze shutdown". */
+    CLEAN,
+
+    /**
+     * The server is shut down as a result of a "blaze clean --expunge.
+     *
+     * <p>In this case, no files should be deleted on shutdown hooks, since clean also deletes the
+     * lock file, and there is a small possibility of the following sequence of events:
+     *
+     * <ol>
+     *   <li> Client 1 runs "blaze clean --expunge"
+     *   <li> Client 2 runs a command and waits for client 1 to finish
+     *   <li> The clean command deletes everything including the lock file
+     *   <li> Client 2 starts running and since the output base is empty, starts up a new server,
+     *     which creates its own socket and PID files
+     *   <li> The server used by client runs its shutdown hooks, deleting the PID files created by
+     *     the new server
+     * </ol>
+     */
+    EXPUNGE,
+  }
+
+  /**
    * By throwing this exception, a command indicates that it wants to shutdown
    * the Blaze server process.
    * See {@link BlazeCommandDispatcher#exec(List, OutErr, long)}.
    */
   public static class ShutdownBlazeServerException extends Exception {
     private final int exitStatus;
+    private final ShutdownMethod method;
 
-    public ShutdownBlazeServerException(int exitStatus, Throwable cause) {
+    public ShutdownBlazeServerException(int exitStatus, ShutdownMethod method, Throwable cause) {
       super(cause);
       this.exitStatus = exitStatus;
+      this.method = method;
     }
 
-    public ShutdownBlazeServerException(int exitStatus) {
+    public ShutdownBlazeServerException(int exitStatus, ShutdownMethod method) {
+      Preconditions.checkState(method != ShutdownMethod.NONE);
       this.exitStatus = exitStatus;
+      this.method = method;
     }
 
     public int getExitStatus() {
       return exitStatus;
     }
+
+    public ShutdownMethod getMethod() {
+      return method;
+    }
   }
 
   private final BlazeRuntime runtime;
@@ -439,7 +476,7 @@ public class BlazeCommandDispatcher {
       numericExitCode = e instanceof OutOfMemoryError
           ? ExitCode.OOM_ERROR.getNumericExitCode()
           : ExitCode.BLAZE_INTERNAL_ERROR.getNumericExitCode();
-      throw new ShutdownBlazeServerException(numericExitCode, e);
+      throw new ShutdownBlazeServerException(numericExitCode, ShutdownMethod.CLEAN, e);
     } finally {
       runtime.afterCommand(env, numericExitCode);
       // Swallow IOException, as we are already in a finally clause
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 f1560b4ba5..52e15f26cf 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
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.runtime;
 
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownMethod;
 import com.google.devtools.build.lib.server.ServerCommand;
 import com.google.devtools.build.lib.util.io.OutErr;
 
@@ -29,12 +30,12 @@ import java.util.logging.Logger;
 public class CommandExecutor implements ServerCommand {
   private static final Logger LOG = Logger.getLogger(CommandExecutor.class.getName());
 
-  private boolean shutdown;
+  private ShutdownMethod shutdown;
   private final BlazeRuntime runtime;
   private final BlazeCommandDispatcher dispatcher;
 
   CommandExecutor(BlazeRuntime runtime, BlazeCommandDispatcher dispatcher) {
-    this.shutdown = false;
+    this.shutdown = ShutdownMethod.NONE;
     this.runtime = runtime;
     this.dispatcher = dispatcher;
   }
@@ -55,7 +56,7 @@ public class CommandExecutor implements ServerCommand {
         writer.flush();
         LOG.severe(message.toString());
       }
-      shutdown = true;
+      shutdown = e.getMethod();
       runtime.shutdown();
       dispatcher.shutdown();
       return e.getExitStatus();
@@ -63,7 +64,7 @@ public class CommandExecutor implements ServerCommand {
   }
 
   @Override
-  public boolean shutdown() {
+  public ShutdownMethod shutdown() {
     return shutdown;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
index 5ebac2c953..1080c4594b 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.buildtool.OutputDirectoryLinksUtils;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.runtime.BlazeCommand;
 import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownBlazeServerException;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownMethod;
 import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.shell.CommandException;
@@ -48,7 +49,6 @@ import java.util.logging.Logger;
          // TODO(bazel-team): Remove this - we inherit a huge number of unused options.
          inherits = { BuildCommand.class })
 public final class CleanCommand implements BlazeCommand {
-
   /**
    * An interface for special options for the clean command.
    */
@@ -177,7 +177,7 @@ public final class CleanCommand implements BlazeCommand {
         env.getWorkspaceName(), env.getWorkspace(), env.getReporter(), symlinkPrefix);
     // shutdown on expunge cleans
     if (cleanOptions.expunge || cleanOptions.expunge_async) {
-      throw new ShutdownBlazeServerException(0);
+      throw new ShutdownBlazeServerException(0, ShutdownMethod.EXPUNGE);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/ShutdownCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/ShutdownCommand.java
index 640602d168..766bc002ab 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/ShutdownCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/ShutdownCommand.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.runtime.commands;
 
 import com.google.devtools.build.lib.runtime.BlazeCommand;
 import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownBlazeServerException;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownMethod;
 import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.util.ExitCode;
@@ -63,7 +64,7 @@ public final class ShutdownCommand implements BlazeCommand {
 
     if (limit == 0 ||
         Runtime.getRuntime().totalMemory() > limit * 1000L * 1000) {
-      throw new ShutdownBlazeServerException(0);
+      throw new ShutdownBlazeServerException(0, ShutdownMethod.CLEAN);
     }
     return ExitCode.SUCCESS;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/server/AfUnixServer.java b/src/main/java/com/google/devtools/build/lib/server/AfUnixServer.java
index 558b87569a..f615d04cd7 100644
--- a/src/main/java/com/google/devtools/build/lib/server/AfUnixServer.java
+++ b/src/main/java/com/google/devtools/build/lib/server/AfUnixServer.java
@@ -18,6 +18,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.common.io.ByteStreams;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownMethod;
 import com.google.devtools.build.lib.server.RPCService.UnknownCommandException;
 import com.google.devtools.build.lib.unix.LocalClientSocket;
 import com.google.devtools.build.lib.unix.LocalServerSocket;
@@ -225,8 +226,16 @@ public final class AfUnixServer extends RPCServer {
               }
             }
             requestIo.shutdown();
-            if (rpcService.isShutdown()) {
-              return;
+            switch (rpcService.getShutdown()) {
+              case NONE:
+                break;
+
+              case CLEAN:
+                return;
+
+              case EXPUNGE:
+                disableShutdownHooks();
+                return;
             }
           }
         } catch (EOFException e) {
@@ -238,7 +247,7 @@ public final class AfUnixServer extends RPCServer {
         }
       }
     } finally {
-      rpcService.shutdown();
+      rpcService.shutdown(ShutdownMethod.CLEAN);
       LOG.info("Logging finished");
     }
   }
@@ -413,7 +422,7 @@ public final class AfUnixServer extends RPCServer {
       LOG.severe("SERVER ERROR: " + trace);
     }
 
-    if (rpcService.isShutdown()) {
+    if (rpcService.getShutdown() != ShutdownMethod.NONE) {
       // In case of shutdown, disable the listening socket *before* we write
       // the last part of the response.  Otherwise, a sufficiently fast client
       // could read the response and exit, and a new client could make a
@@ -533,10 +542,6 @@ public final class AfUnixServer extends RPCServer {
                                         Path workspaceDir,
                                         int maxIdleSeconds)
       throws IOException {
-    if (!serverDirectory.exists()) {
-      serverDirectory.createDirectory();
-    }
-
     // Creates and starts the RPC server.
     RPCService service = new RPCService(appCommand);
 
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 f740c9e57d..a621314c04 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
@@ -393,8 +393,18 @@ public class GrpcServerImpl extends RPCServer implements CommandServerGrpc.Comma
     observer.onNext(response);
     observer.onCompleted();
 
-    if (commandExecutor.shutdown()) {
-      server.shutdownNow();
+    switch (commandExecutor.shutdown()) {
+      case NONE:
+        break;
+
+      case CLEAN:
+        server.shutdownNow();
+        break;
+
+      case EXPUNGE:
+        disableShutdownHooks();
+        server.shutdownNow();
+        break;
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/server/RPCServer.java b/src/main/java/com/google/devtools/build/lib/server/RPCServer.java
index ad7c9d1377..1e0f8f2b66 100644
--- a/src/main/java/com/google/devtools/build/lib/server/RPCServer.java
+++ b/src/main/java/com/google/devtools/build/lib/server/RPCServer.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.StringWriter;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Logger;
 
 /**
@@ -27,6 +28,7 @@ import java.util.logging.Logger;
  */
 public abstract class RPCServer {
   private static final Logger LOG = Logger.getLogger(RPCServer.class.getName());
+  private static AtomicBoolean runShutdownHooks = new AtomicBoolean(true);
 
   /**
    * Factory class for the gRPC server.
@@ -48,17 +50,25 @@ public abstract class RPCServer {
     RPCServer.deleteAtExit(pidSymlink, /*deleteParent=*/ false);
   }
 
+  protected void disableShutdownHooks() {
+    runShutdownHooks.set(false);
+  }
+
   /**
    * Schedule the specified file for (attempted) deletion at JVM exit.
    */
-  protected static void deleteAtExit(final Path socketFile, final boolean deleteParent) {
+  protected static void deleteAtExit(final Path path, final boolean deleteParent) {
     Runtime.getRuntime().addShutdownHook(new Thread() {
         @Override
         public void run() {
+          if (!runShutdownHooks.get()) {
+            return;
+          }
+
           try {
-            socketFile.delete();
+            path.delete();
             if (deleteParent) {
-              socketFile.getParentDirectory().delete();
+              path.getParentDirectory().delete();
             }
           } catch (IOException e) {
             printStack(e);
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 ab40553b31..73a246520a 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
@@ -16,6 +16,8 @@ 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.runtime.BlazeCommandDispatcher.ShutdownMethod;
+import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.OutErr;
 
 import java.util.List;
@@ -32,9 +34,9 @@ import java.util.logging.Logger;
  */
 public final class RPCService {
 
-  private boolean isShutdown;
   private static final Logger LOG = Logger.getLogger(RPCService.class.getName());
   private final ServerCommand appCommand;
+  private ShutdownMethod shutdown = ShutdownMethod.NONE;
 
   public RPCService(ServerCommand appCommand) {
     this.appCommand = appCommand;
@@ -58,7 +60,7 @@ public final class RPCService {
   public int executeRequest(List<String> request,
                             OutErr outErr,
                             long firstContactTime) throws Exception {
-    if (isShutdown) {
+    if (shutdown != ShutdownMethod.NONE) {
       throw new IllegalStateException("Received request after shutdown.");
     }
     String command = Iterables.getFirst(request, "");
@@ -68,8 +70,9 @@ public final class RPCService {
       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();
+      ShutdownMethod commandShutdown = appCommand.shutdown();
+      if (commandShutdown != ShutdownMethod.NONE) {  // an application shutdown request
+        shutdown(commandShutdown);
       }
       return result;
     } else {
@@ -79,14 +82,15 @@ public final class RPCService {
 
   /**
    * After executing this function, further requests will fail, and
-   * {@link #isShutdown()} will return true.
+   * {@link #getShutdown()} will the shutdown method passed in.
    */
-  public void shutdown() {
-    if (isShutdown) {
+  public void shutdown(ShutdownMethod method) {
+    Preconditions.checkState(method != ShutdownMethod.NONE);
+    if (shutdown != ShutdownMethod.NONE) {
       return;
     }
     LOG.info("RPC Service: shutting down ...");
-    isShutdown = true;
+    shutdown = method;
   }
 
   /**
@@ -94,8 +98,8 @@ public final class RPCService {
    * {@link #executeRequest(List, OutErr, long)} will result in an
    * {@link IllegalStateException}
    */
-  public boolean isShutdown() {
-    return isShutdown;
+  public ShutdownMethod getShutdown() {
+    return 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 577cb4871f..e2d6d85d72 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
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.server;
 
 import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownMethod;
 import com.google.devtools.build.lib.util.io.OutErr;
 
 import java.util.List;
@@ -32,9 +33,8 @@ public interface ServerCommand {
       String clientDescription, long firstContactTime) throws InterruptedException;
 
   /**
-   * The implementation returns true from this method to initiate a shutdown.
-   * No further requests will be handled.
+   * Whether the server needs to be shutdown, and if so, in what manner.
    */
-  boolean shutdown();
+  ShutdownMethod shutdown();
 
 }