Move pid file watcher to its own class
Declutters GrpcServerImpl, makes it easier to test in isolation, and makes the
pid more readily available for other things that may want it.
The @VisibleForTesting arrangement is slightly awkward, but I think it's worth
it to have the test coverage given the number of times I messed up this
refactoring.
PiperOrigin-RevId: 328986378
diff --git a/src/main/java/com/google/devtools/build/lib/server/BUILD b/src/main/java/com/google/devtools/build/lib/server/BUILD
index 45d56d0..172b145 100644
--- a/src/main/java/com/google/devtools/build/lib/server/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/server/BUILD
@@ -16,6 +16,7 @@
"CommandManager.java",
"GrpcServerImpl.java",
"IdleServerTasks.java",
+ "PidFileWatcher.java",
"ServerWatcherRunnable.java",
],
deps = [
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 ee2df4a..afc218f 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
@@ -20,7 +20,6 @@
import com.google.common.flogger.GoogleLogger;
import com.google.common.net.InetAddresses;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
-import com.google.common.util.concurrent.Uninterruptibles;
import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.clock.Clock;
import com.google.devtools.build.lib.runtime.BlazeCommandResult;
@@ -74,7 +73,6 @@
import java.util.Optional;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
-import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
/**
@@ -275,56 +273,6 @@
}
}
- /**
- * A thread that watches if the PID file changes and shuts down the server immediately if so.
- */
- private class PidFileWatcherThread extends Thread {
- private boolean shuttingDown = false;
-
- private PidFileWatcherThread() {
- super("pid-file-watcher");
- setDaemon(true);
- }
-
- // The synchronized block is here so that if the "PID file deleted" timer kicks in during a
- // regular shutdown, they don't race.
- private synchronized void signalShutdown() {
- shuttingDown = true;
- }
-
- @Override
- public void run() {
- while (true) {
- Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS);
- boolean ok = false;
- try {
- String pidFileContents = new String(FileSystemUtils.readContentAsLatin1(pidFile));
- ok = pidFileContents.equals(pidInFile);
- } catch (IOException e) {
- logger.atInfo().log("Cannot read PID file: %s", e.getMessage());
- // Handled by virtue of ok not being set to true
- }
-
- if (!ok) {
- synchronized (PidFileWatcherThread.this) {
- if (shuttingDown) {
- logger.atWarning().log(
- "PID file deleted or overwritten but shutdown is already in progress");
- break;
- }
-
- shuttingDown = true;
- // Someone overwrote the PID file. Maybe it's another server, so shut down as quickly
- // as possible without even running the shutdown hooks (that would delete it)
- logger.atSevere().log(
- "PID file deleted or overwritten, exiting as quickly as possible");
- Runtime.getRuntime().halt(ExitCode.BLAZE_INTERNAL_ERROR.getNumericExitCode());
- }
- }
- }
- }
- }
-
// These paths are all relative to the server directory
private static final String PORT_FILE = "command_port";
private static final String REQUEST_COOKIE_FILE = "request_cookie";
@@ -341,9 +289,8 @@
private final String requestCookie;
private final String responseCookie;
private final int maxIdleSeconds;
- private final PidFileWatcherThread pidFileWatcherThread;
- private final Path pidFile;
- private final String pidInFile;
+ private final PidFileWatcher pidFileWatcherThread;
+ private final int serverPid;
private final List<Path> filesToDeleteAtExit = new ArrayList<>();
private final int port;
@@ -367,15 +314,8 @@
// server.pid was written in the C++ launcher after fork() but before exec().
// The client only accesses the pid file after connecting to the socket
// which ensures that it gets the correct pid value.
- pidFile = serverDirectory.getRelative("server.pid.txt");
- try {
- pidInFile = new String(FileSystemUtils.readContentAsLatin1(pidFile));
- } catch (IOException e) {
- throw createFilesystemFailureException(
- "Server pid file read failed: " + e.getMessage(),
- Code.SERVER_PID_TXT_FILE_READ_FAILURE,
- e);
- }
+ Path pidFile = serverDirectory.getRelative("server.pid.txt");
+ serverPid = readPidFile(pidFile);
deleteAtExit(pidFile);
this.dispatcher = dispatcher;
@@ -397,7 +337,7 @@
this.requestCookie = requestCookie;
this.responseCookie = responseCookie;
- pidFileWatcherThread = new PidFileWatcherThread();
+ pidFileWatcherThread = new PidFileWatcher(pidFile, serverPid);
pidFileWatcherThread.start();
commandManager = new CommandManager(doIdleServerTasks);
}
@@ -512,7 +452,7 @@
ServerInfo info =
ServerInfo.newBuilder()
- .setPid(Integer.parseInt(pidInFile))
+ .setPid(serverPid)
.setAddress(addressString)
.setRequestCookie(requestCookie)
.setResponseCookie(responseCookie)
@@ -765,6 +705,23 @@
}
}
+ private static int readPidFile(Path pidFile) throws AbruptExitException {
+ try {
+ return Integer.parseInt(new String(FileSystemUtils.readContentAsLatin1(pidFile)));
+ } catch (IOException e) {
+ throw createFilesystemFailureException(
+ "Server pid file read failed: " + e.getMessage(),
+ Code.SERVER_PID_TXT_FILE_READ_FAILURE,
+ e);
+ } catch (NumberFormatException e) {
+ // Invalid contents (not a number) is more likely than not a filesystem issue.
+ throw createFilesystemFailureException(
+ "Server pid file corrupted: " + e.getMessage(),
+ Code.SERVER_PID_TXT_FILE_READ_FAILURE,
+ new IOException(e));
+ }
+ }
+
private static AbruptExitException createFilesystemFailureException(
String message, Code detailedCode, IOException e) {
return new AbruptExitException(
diff --git a/src/main/java/com/google/devtools/build/lib/server/PidFileWatcher.java b/src/main/java/com/google/devtools/build/lib/server/PidFileWatcher.java
new file mode 100644
index 0000000..db3b87b
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/server/PidFileWatcher.java
@@ -0,0 +1,113 @@
+// Copyright 2020 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.server;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.flogger.GoogleLogger;
+import com.google.common.util.concurrent.Uninterruptibles;
+import com.google.devtools.build.lib.util.ExitCode;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import java.io.IOException;
+import javax.annotation.concurrent.GuardedBy;
+
+/** A thread that watches if the PID file changes and shuts down the server immediately if so. */
+class PidFileWatcher extends Thread {
+ private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
+
+ private final Path pidFile;
+ private final int serverPid;
+ private final Runnable onPidFileChange;
+
+ @GuardedBy("this")
+ private boolean shuttingDown = false;
+
+ PidFileWatcher(Path pidFile, int serverPid) {
+ this(
+ pidFile,
+ serverPid,
+ () -> Runtime.getRuntime().halt(ExitCode.BLAZE_INTERNAL_ERROR.getNumericExitCode()));
+ }
+
+ /** Test-only constructor allowing a more gentle {@link #onPidFileChange} callback. */
+ @VisibleForTesting
+ PidFileWatcher(Path pidFile, int serverPid, Runnable onPidFileChange) {
+ super("pid-file-watcher");
+ this.pidFile = pidFile;
+ this.serverPid = serverPid;
+ this.onPidFileChange = onPidFileChange;
+ setDaemon(true);
+ }
+
+ /**
+ * Signals a shutdown is in progress, thus pid file changes can be expected and should not trigger
+ * an aggressive shutdown.
+ */
+ synchronized void signalShutdown() {
+ shuttingDown = true;
+ }
+
+ @Override
+ public void run() {
+ do {
+ Uninterruptibles.sleepUninterruptibly(5, SECONDS);
+ } while (runPidFileChecks());
+ }
+
+ /**
+ * Runs one iteration of pid file checks. Returns whether or not the main loop should continue, or
+ * crashes the program via {@link #onPidFileChange} callback.
+ */
+ @VisibleForTesting
+ boolean runPidFileChecks() {
+ if (pidFileValid(pidFile, serverPid)) {
+ return true;
+ }
+
+ synchronized (this) {
+ if (shuttingDown) {
+ logger.atWarning().log(
+ "PID file deleted or overwritten but shutdown is already in progress");
+ return false;
+ }
+
+ shuttingDown = true;
+ // Someone overwrote the PID file. Maybe it's another server, so shut down as quickly
+ // as possible without even running the shutdown hooks (that would delete it)
+ logger.atSevere().log("PID file deleted or overwritten, exiting as quickly as possible");
+ onPidFileChange.run();
+ throw new IllegalStateException("Should have crashed.");
+ }
+ }
+
+ private static boolean pidFileValid(Path pidFile, int expectedPid) {
+ String pidFileContents;
+ try {
+ pidFileContents = new String(FileSystemUtils.readContentAsLatin1(pidFile));
+ } catch (IOException e) {
+ logger.atInfo().log("Cannot read PID file: %s", e.getMessage());
+ return false;
+ }
+
+ try {
+ return Integer.parseInt(pidFileContents) == expectedPid;
+ } catch (NumberFormatException e) {
+ logger.atWarning().withCause(e).log("Pid was invalid: %s", pidFileContents);
+ return false;
+ }
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/server/PidFileWatcherTest.java b/src/test/java/com/google/devtools/build/lib/server/PidFileWatcherTest.java
new file mode 100644
index 0000000..5d43f9e
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/server/PidFileWatcherTest.java
@@ -0,0 +1,103 @@
+// Copyright 2020 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.server;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.nio.charset.StandardCharsets.US_ASCII;
+import static org.junit.Assert.assertThrows;
+
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.io.IOException;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link PidFileWatcher}. */
+@RunWith(JUnit4.class)
+public class PidFileWatcherTest {
+
+ private static final int EXPECTED_PID = 42;
+ private static final IllegalStateException THROWN_ON_HALT = new IllegalStateException("crash!");
+
+ private Path pidFile;
+ private PidFileWatcher underTest;
+
+ @Before
+ public void setUp() {
+ FileSystem fileSystem = new InMemoryFileSystem();
+ pidFile = fileSystem.getPath("/pid");
+ underTest =
+ new PidFileWatcher(
+ pidFile,
+ EXPECTED_PID,
+ () -> {
+ throw THROWN_ON_HALT;
+ });
+ }
+
+ @Test
+ public void testMissingPidFileHaltsProgram() throws IOException {
+ // Delete just in case.
+ pidFile.delete();
+
+ assertPidCheckHaltsProgram();
+ }
+
+ @Test
+ public void testEmptyPidFileCountsAsChanged() throws IOException {
+ FileSystemUtils.writeContent(pidFile, new byte[0]);
+
+ assertPidCheckHaltsProgram();
+ }
+
+ @Test
+ public void testGarbagePidFileCountsAsChanged() throws IOException {
+ FileSystemUtils.writeContent(pidFile, "junk".getBytes(US_ASCII));
+
+ assertPidCheckHaltsProgram();
+ }
+
+ @Test
+ public void testPidFileContinuesExecution() throws IOException {
+ FileSystemUtils.writeContent(pidFile, "42".getBytes(US_ASCII));
+
+ assertThat(underTest.runPidFileChecks()).isTrue();
+ }
+
+ @Test
+ public void testPidFileTrailingWhitespaceNotTolerated() throws IOException {
+ FileSystemUtils.writeContent(pidFile, "42\n".getBytes(US_ASCII));
+
+ assertPidCheckHaltsProgram();
+ }
+
+ @Test
+ public void testPidFileChangeAfterShutdownNotificationStopsWatcher() throws IOException {
+ FileSystemUtils.writeContent(pidFile, "42\n".getBytes(US_ASCII));
+
+ underTest.signalShutdown();
+ assertThat(underTest.runPidFileChecks()).isFalse();
+ }
+
+ private void assertPidCheckHaltsProgram() {
+ IllegalStateException expected =
+ assertThrows(IllegalStateException.class, underTest::runPidFileChecks);
+ assertThat(expected).isSameInstanceAs(THROWN_ON_HALT);
+ }
+}
