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

2 files changed, 22 insertions, 14 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/server/AfUnixServerTest.java b/src/test/java/com/google/devtools/build/lib/server/AfUnixServerTest.java
index 89c046e653..1d712e1d4c 100644
--- a/src/test/java/com/google/devtools/build/lib/server/AfUnixServerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/server/AfUnixServerTest.java
@@ -20,6 +20,7 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.LockingMode;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownMethod;
 import com.google.devtools.build.lib.testutil.Suite;
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.util.JavaClock;
@@ -72,9 +73,10 @@ public class AfUnixServerTest {
       outErr.printErr(COMMAND_STDERR);
       return 42;
     }
+
     @Override
-    public boolean shutdown() {
-      return false;
+    public ShutdownMethod shutdown() {
+      return ShutdownMethod.NONE;
     }
   };
 
diff --git a/src/test/java/com/google/devtools/build/lib/server/RPCServiceTest.java b/src/test/java/com/google/devtools/build/lib/server/RPCServiceTest.java
index 93d688e619..1791f64db5 100644
--- a/src/test/java/com/google/devtools/build/lib/server/RPCServiceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/server/RPCServiceTest.java
@@ -16,11 +16,10 @@ package com.google.devtools.build.lib.server;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.LockingMode;
+import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.ShutdownMethod;
 import com.google.devtools.build.lib.server.RPCService.UnknownCommandException;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.util.io.RecordingOutErr;
@@ -48,8 +47,8 @@ public class RPCServiceTest {
       return 42;
     }
     @Override
-    public boolean shutdown() {
-      return false;
+    public ShutdownMethod shutdown() {
+      return ShutdownMethod.NONE;
     }
   };
 
@@ -91,8 +90,8 @@ public class RPCServiceTest {
               return 0;
             }
             @Override
-            public boolean shutdown() {
-              return false;
+            public ShutdownMethod shutdown() {
+              return ShutdownMethod.NONE;
             }
           });
 
@@ -104,17 +103,24 @@ public class RPCServiceTest {
 
   @Test
   public void testShutdownState() throws Exception {
-    assertFalse(service.isShutdown());
-    service.shutdown();
-    assertTrue(service.isShutdown());
-    service.shutdown();
-    assertTrue(service.isShutdown());
+    assertEquals(ShutdownMethod.NONE, service.getShutdown());
+    service.shutdown(ShutdownMethod.CLEAN);
+    assertEquals(ShutdownMethod.CLEAN, service.getShutdown());
+    service.shutdown(ShutdownMethod.EXPUNGE);
+    assertEquals(ShutdownMethod.CLEAN, service.getShutdown());
+  }
+
+  @Test
+  public void testExpungeShutdown() throws Exception {
+    assertEquals(ShutdownMethod.NONE, service.getShutdown());
+    service.shutdown(ShutdownMethod.EXPUNGE);
+    assertEquals(ShutdownMethod.EXPUNGE, service.getShutdown());
   }
 
   @Test
   public void testCommandFailsAfterShutdown() throws Exception {
     RecordingOutErr outErr = new RecordingOutErr();
-    service.shutdown();
+    service.shutdown(ShutdownMethod.CLEAN);
     try {
       service.executeRequest(Arrays.asList("blaze"), outErr, 0);
       fail();