Fix memory leak of Reporter through System.err.
During the lifetime of a Blaze command, we patch System.out and System.err with custom PrintStream instances (containing a ReporterStream). ReporterStream transitively retains some expensive objects, notably anything registered on the EventBus. Anyone is free to read and retain our PrintStream wrapping ReporterStream via System.out or System.err until the command completes.
This change uses an additional layer of control to switch back to the default System.out and System.err and free the custom streams, even if someone retains a reference.
RELNOTES: None.
PiperOrigin-RevId: 255754558
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 290ae1c..46d6b28 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
@@ -53,7 +53,6 @@
import java.io.BufferedOutputStream;
import java.io.IOException;
import java.io.OutputStream;
-import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
@@ -307,13 +306,12 @@
}
BlazeCommandResult result = BlazeCommandResult.exitCode(ExitCode.BLAZE_INTERNAL_ERROR);
- PrintStream savedOut = System.out;
- PrintStream savedErr = System.err;
boolean afterCommandCalled = false;
- try {
+ Reporter reporter = env.getReporter();
+ try (OutErr.SystemPatcher systemOutErrPatcher = reporter.getOutErr().getSystemPatcher()) {
// Temporary: there are modules that output events during beforeCommand, but the reporter
// isn't setup yet. Add the stored event handler to catch those events.
- env.getReporter().addHandler(storedEventHandler);
+ reporter.addHandler(storedEventHandler);
for (BlazeModule module : runtime.getBlazeModules()) {
try (SilentCloseable closeable = Profiler.instance().profile(module + ".beforeCommand")) {
module.beforeCommand(env);
@@ -328,7 +326,7 @@
earlyExitCode = e.getExitCode();
}
}
- env.getReporter().removeHandler(storedEventHandler);
+ reporter.removeHandler(storedEventHandler);
// Setup stdout / stderr.
outErr = tee(outErr, env.getOutputListeners());
@@ -346,7 +344,6 @@
commandName, firstContactTime, false, true, env.getCommandId().toString()));
}
- Reporter reporter = env.getReporter();
try (SilentCloseable closeable = Profiler.instance().profile("setup event handler")) {
BlazeCommandEventHandler.Options eventHandlerOptions =
options.getOptions(BlazeCommandEventHandler.Options.class);
@@ -455,11 +452,9 @@
}
}
- // While a Blaze command is active, direct all errors to the client's
- // event handler (and out/err streams).
- OutErr reporterOutErr = reporter.getOutErr();
- System.setOut(new PrintStream(reporterOutErr.getOutputStream(), /*autoflush=*/ true));
- System.setErr(new PrintStream(reporterOutErr.getErrorStream(), /*autoflush=*/ true));
+ // While a Blaze command is active, direct all errors to the client's event handler (and
+ // out/err streams).
+ systemOutErrPatcher.start();
try (SilentCloseable closeable = Profiler.instance().profile("CommandEnv.beforeCommand")) {
// Notify the BlazeRuntime, so it can do some initial setup.
@@ -494,11 +489,10 @@
env.setupPackageCache(options);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
- env.getReporter()
- .handle(Event.error("command interrupted while setting up package cache"));
+ reporter.handle(Event.error("command interrupted while setting up package cache"));
earlyExitCode = ExitCode.INTERRUPTED;
} catch (AbruptExitException e) {
- env.getReporter().handle(Event.error(e.getMessage()));
+ reporter.handle(Event.error(e.getMessage()));
earlyExitCode = e.getExitCode();
}
if (!earlyExitCode.equals(ExitCode.SUCCESS)) {
@@ -553,8 +547,6 @@
Flushables.flushQuietly(outErr.getOutputStream());
Flushables.flushQuietly(outErr.getErrorStream());
- System.setOut(savedOut);
- System.setErr(savedErr);
env.getTimestampGranularityMonitor().waitForTimestampGranularity(outErr);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/util/io/OutErr.java b/src/main/java/com/google/devtools/build/lib/util/io/OutErr.java
index bad0c91..b4c12b9 100644
--- a/src/main/java/com/google/devtools/build/lib/util/io/OutErr.java
+++ b/src/main/java/com/google/devtools/build/lib/util/io/OutErr.java
@@ -21,8 +21,7 @@
import java.io.PrintWriter;
/**
- * A pair of output streams to be used for redirecting the output and error
- * streams of a subprocess.
+ * A pair of output streams to be used for redirecting the output and error streams of a subprocess.
*/
public class OutErr implements Closeable {
@@ -52,19 +51,56 @@
}
/**
- * This method redirects {@link System#out} / {@link System#err} into
- * {@code this} object. After calling this method, writing to
- * {@link System#out} or {@link System#err} will result in
- * {@code "System.out: " + message} or {@code "System.err: " + message}
- * being written to the OutputStreams of {@code this} instance.
+ * Temporarily patches {@link System#out} and {@link System#err} with custom streams.
*
- * Note: This method affects global variables.
+ * <p>{@link #start} is called to signal the beginning of the scope of the patch. {@link #close}
+ * ends the scope of the patch, returning {@link System#out} and {@link System#err} to what they
+ * were before.
*/
- public void addSystemOutErrAsSource() {
- System.setOut(new PrintStream(new LinePrefixingOutputStream("System.out: ", getOutputStream()),
- /*autoflush=*/false));
- System.setErr(new PrintStream(new LinePrefixingOutputStream("System.err: ", getErrorStream()),
- /*autoflush=*/false));
+ public interface SystemPatcher extends AutoCloseable {
+ void start();
+ }
+
+ /** Returns a {@link SystemPatcher} that uses this instance's out and err streams. */
+ public final SystemPatcher getSystemPatcher() {
+ PrintStream savedOut = System.out;
+ PrintStream savedErr = System.err;
+ SwitchingPrintStream outPatch = new SwitchingPrintStream(out);
+ SwitchingPrintStream errPatch = new SwitchingPrintStream(err);
+ return new SystemPatcher() {
+ @Override
+ public void start() {
+ System.setOut(outPatch);
+ System.setErr(errPatch);
+ }
+
+ @Override
+ public void close() {
+ System.setOut(savedOut);
+ System.setErr(savedErr);
+ outPatch.switchBackTo(savedOut);
+ errPatch.switchBackTo(savedErr);
+ }
+ };
+ }
+
+ /**
+ * Starts by streaming to {@code override}, then switches back to {@code saved}.
+ *
+ * <p>The switching strategy is used to guard against memory leaks. For example, if {@code
+ * override} is passed directly to {@link System#setErr}, anyone may retain a reference to it via
+ * {@link System#err}. Instead, they will get a reference to this class, which frees up {@code
+ * override} in {@link #switchBackTo}.
+ */
+ private static final class SwitchingPrintStream extends PrintStream {
+
+ private SwitchingPrintStream(OutputStream override) {
+ super(override, /*autoFlush=*/ true);
+ }
+
+ private void switchBackTo(OutputStream saved) {
+ out = saved;
+ }
}
/**
