From f6017af8b4f05979f9d53472931f56ac7274a586 Mon Sep 17 00:00:00 2001 From: Luca Di Grazia Date: Sun, 4 Sep 2022 17:33:54 +0200 Subject: [PATCH] Use isIPv6Preferred to check if we should use ipv6 or not Follow up on: https://github.com/bazelbuild/bazel/pull/11776 Fixes https://github.com/bazelbuild/bazel/issues/11756 RELNOTES: None PiperOrigin-RevId: 321541781 --- .../build/lib/server/GrpcServerImpl.java | 225 +++++++++++++----- 1 file changed, 166 insertions(+), 59 deletions(-) diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java index 66890224ff9..ee2df4a1e6b 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java @@ -20,12 +20,13 @@ 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; +import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.CommandDispatcher; import com.google.devtools.build.lib.runtime.CommandDispatcher.LockingMode; -import com.google.devtools.build.lib.runtime.SafeRequestLogging; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; import com.google.devtools.build.lib.server.CommandManager.RunningCommand; import com.google.devtools.build.lib.server.CommandProtos.CancelRequest; @@ -63,12 +64,18 @@ import io.netty.channel.unix.Socket; import java.io.IOException; import java.io.OutputStream; +import java.io.PrintWriter; +import java.io.StringWriter; import java.net.InetSocketAddress; import java.nio.charset.StandardCharsets; import java.security.SecureRandom; +import java.util.ArrayList; +import java.util.List; 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; /** * gRPC server class. @@ -102,35 +109,38 @@ */ public class GrpcServerImpl extends CommandServerGrpc.CommandServerImplBase implements RPCServer { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + private final boolean shutdownOnLowSysMem; - public static GrpcServerImpl create( - CommandDispatcher dispatcher, - ShutdownHooks shutdownHooks, - PidFileWatcher pidFileWatcher, - Clock clock, - int port, - Path serverDirectory, - int serverPid, - int maxIdleSeconds, - boolean shutdownOnLowSysMem, - boolean idleServerTasks) { - SecureRandom random = new SecureRandom(); - return new GrpcServerImpl( - dispatcher, - shutdownHooks, - pidFileWatcher, - clock, - port, - generateCookie(random, 16), - generateCookie(random, 16), - serverDirectory, - serverPid, - maxIdleSeconds, - shutdownOnLowSysMem, - idleServerTasks); + /** + * Factory class. Instantiated by reflection. + * + *

Used so that method calls using reflection are as simple as possible. + */ + public static class Factory implements RPCServer.Factory { + @Override + public RPCServer create( + CommandDispatcher dispatcher, + Clock clock, + int port, + Path serverDirectory, + int maxIdleSeconds, + boolean shutdownOnLowSysMem, + boolean idleServerTasks) + throws AbruptExitException { + SecureRandom random = new SecureRandom(); + return new GrpcServerImpl( + dispatcher, + clock, + port, + generateCookie(random, 16), + generateCookie(random, 16), + serverDirectory, + maxIdleSeconds, + shutdownOnLowSysMem, + idleServerTasks); + } } - @VisibleForTesting enum StreamType { STDOUT, @@ -265,25 +275,76 @@ public void write(int byteAsInt) throws IOException { } } + /** + * 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"; private static final String RESPONSE_COOKIE_FILE = "response_cookie"; private static final String SERVER_INFO_FILE = "server_info.rawproto"; + private static final AtomicBoolean runShutdownHooks = new AtomicBoolean(true); private final CommandManager commandManager; private final CommandDispatcher dispatcher; private final Executor commandExecutorPool; - private final ShutdownHooks shutdownHooks; private final Clock clock; private final Path serverDirectory; private final String requestCookie; private final String responseCookie; private final int maxIdleSeconds; - private final boolean shutdownOnLowSysMem; - private final PidFileWatcher pidFileWatcher; - private final int serverPid; + private final PidFileWatcherThread pidFileWatcherThread; + private final Path pidFile; + private final String pidInFile; + private final List filesToDeleteAtExit = new ArrayList<>(); private final int port; private Server server; @@ -292,29 +353,35 @@ public void write(int byteAsInt) throws IOException { @VisibleForTesting GrpcServerImpl( CommandDispatcher dispatcher, - ShutdownHooks shutdownHooks, - PidFileWatcher pidFileWatcher, Clock clock, int port, String requestCookie, String responseCookie, Path serverDirectory, - int serverPid, int maxIdleSeconds, boolean shutdownOnLowSysMem, - boolean doIdleServerTasks) { - this.dispatcher = dispatcher; - this.shutdownHooks = shutdownHooks; - this.pidFileWatcher = pidFileWatcher; + boolean doIdleServerTasks) + throws AbruptExitException { + Runtime.getRuntime().addShutdownHook(new Thread(() -> shutdownHook())); + + // 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); + } + deleteAtExit(pidFile); + this.dispatcher = dispatcher; this.clock = clock; - this.port = port; - this.requestCookie = requestCookie; - this.responseCookie = responseCookie; - this.serverDirectory = serverDirectory; - this.serverPid = serverPid; - + this.port = port; this.maxIdleSeconds = maxIdleSeconds; this.shutdownOnLowSysMem = shutdownOnLowSysMem; this.serving = false; @@ -327,7 +394,11 @@ public void write(int byteAsInt) throws IOException { .setDaemon(true) .build())); + this.requestCookie = requestCookie; + this.responseCookie = responseCookie; + pidFileWatcherThread = new PidFileWatcherThread(); + pidFileWatcherThread.start(); commandManager = new CommandManager(doIdleServerTasks); } @@ -363,8 +434,8 @@ private static String generateCookie(SecureRandom random, int byteCount) { */ @Override public void prepareForAbruptShutdown() { - shutdownHooks.disable(); - pidFileWatcher.signalShutdown(); + disableShutdownHooks(); + pidFileWatcherThread.signalShutdown(); } @Override @@ -388,13 +459,7 @@ public void serve() throws AbruptExitException { throw new IOException("ipv6 is not preferred on the system."); } server = - NettyServerBuilder.forAddress(address) - .addService(this) - .directExecutor() - // disable auto flow control https://github.com/bazelbuild/bazel/issues/12264 - .flowControlWindow(NettyServerBuilder.DEFAULT_FLOW_CONTROL_WINDOW) - .build() - .start(); + NettyServerBuilder.forAddress(address).addService(this).directExecutor().build().start(); } catch (IOException ipv6Exception) { address = new InetSocketAddress("127.0.0.1", port); try { @@ -402,8 +467,6 @@ public void serve() throws AbruptExitException { NettyServerBuilder.forAddress(address) .addService(this) .directExecutor() - // disable auto flow control https://github.com/bazelbuild/bazel/issues/12264 - .flowControlWindow(NettyServerBuilder.DEFAULT_FLOW_CONTROL_WINDOW) .build() .start(); } catch (IOException ipv4Exception) { @@ -449,7 +512,7 @@ private void writeServerStatusFiles(InetSocketAddress address) throws AbruptExit ServerInfo info = ServerInfo.newBuilder() - .setPid(serverPid) + .setPid(Integer.parseInt(pidInFile)) .setAddress(addressString) .setRequestCookie(requestCookie) .setResponseCookie(responseCookie) @@ -463,7 +526,7 @@ private void writeServerStatusFiles(InetSocketAddress address) throws AbruptExit } Path serverInfoFile = serverDirectory.getChild(SERVER_INFO_FILE); serverInfoTmpFile.renameTo(serverInfoFile); - shutdownHooks.deleteAtExit(serverInfoFile); + deleteAtExit(serverInfoFile); } catch (IOException e) { throw createFilesystemFailureException( "Failed to write server info file: " + e.getMessage(), Code.SERVER_FILE_WRITE_FAILURE, e); @@ -480,7 +543,51 @@ private void writeServerFile(String name, String contents) throws AbruptExitExce Code.SERVER_FILE_WRITE_FAILURE, e); } - shutdownHooks.deleteAtExit(file); + deleteAtExit(file); + } + + protected void disableShutdownHooks() { + runShutdownHooks.set(false); + } + + private void shutdownHook() { + if (!runShutdownHooks.get()) { + return; + } + + List files; + synchronized (filesToDeleteAtExit) { + files = new ArrayList<>(filesToDeleteAtExit); + } + for (Path path : files) { + try { + path.delete(); + } catch (IOException e) { + printStack(e); + } + } + } + + /** + * Schedule the specified file for (attempted) deletion at JVM exit. + */ + protected void deleteAtExit(final Path path) { + synchronized (filesToDeleteAtExit) { + filesToDeleteAtExit.add(path); + } + } + + static void printStack(IOException e) { + /* + * Hopefully this never happens. It's not very nice to just write this + * to the user's console, but I'm not sure what better choice we have. + */ + StringWriter err = new StringWriter(); + PrintWriter printErr = new PrintWriter(err); + printErr.println("=======[BAZEL SERVER: ENCOUNTERED IO EXCEPTION]======="); + e.printStackTrace(printErr); + printErr.println("====================================================="); + logger.atSevere().log(err.toString()); } private void executeCommand(RunRequest request, BlockingStreamObserver observer) { @@ -549,7 +656,7 @@ private void executeCommand(RunRequest request, BlockingStreamObserver