Consume compiler stderr in the same process

Summary: Since we no longer need to concurrently handle preprocessed output and error message simultaneously, I simplify the code for handling error messages by reading and transforming the lines in the same thread instead of in a different thread. Test Plan: CI Reviewed By: beefon fbshipit-source-id: 6781dab
classicvalues · May 15, 2017 · 54ebcd4 · 54ebcd4
1 parent 3286637
commit 54ebcd4
Show file tree

Hide file tree

Showing 7 changed files with 20 additions and 178 deletions.
diff --git a/.../buck/cxx/CxxErrorTransformerFactory.java → ...acebook/buck/cxx/CxxErrorTransformer.java b/.../buck/cxx/CxxErrorTransformerFactory.java → ...acebook/buck/cxx/CxxErrorTransformer.java
@@ -18,15 +18,10 @@
 
 import com.facebook.buck.io.MorePaths;
 import com.facebook.buck.io.ProjectFilesystem;
-import com.facebook.buck.step.ExecutionContext;
-import com.facebook.buck.step.ExecutorPool;
 import com.facebook.buck.util.Escaper;
-import com.facebook.buck.util.LineProcessorRunnable;
 import com.facebook.buck.util.environment.Platform;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
-import java.io.InputStream;
-import java.io.OutputStream;
 import java.nio.file.Path;
 import java.util.Optional;
 import java.util.regex.Matcher;
@@ -38,7 +33,7 @@
  * <p>When preprocessing/compiling, the compiler may be run in a manner where the emitted paths are
  * inaccurate, this stream transformer rewrite error output to give sensible paths to the user.
  */
-class CxxErrorTransformerFactory {
+class CxxErrorTransformer {
 
   private final ProjectFilesystem filesystem;
   private final boolean shouldAbsolutize;
@@ -48,25 +43,13 @@ class CxxErrorTransformerFactory {
    * @param shouldAbsolutize whether to transform paths to absolute paths.
    * @param pathNormalizer Path replacements to rewrite symlinked C headers.
    */
-  public CxxErrorTransformerFactory(
+  public CxxErrorTransformer(
       ProjectFilesystem filesystem, boolean shouldAbsolutize, HeaderPathNormalizer pathNormalizer) {
     this.filesystem = filesystem;
     this.shouldAbsolutize = shouldAbsolutize;
     this.pathNormalizer = pathNormalizer;
   }
 
-  /** Create a thread to process lines in the stream asynchronously. */
-  public LineProcessorRunnable createTransformerThread(
-      ExecutionContext context, InputStream inputStream, OutputStream outputStream) {
-    return new LineProcessorRunnable(
-        context.getExecutorService(ExecutorPool.CPU), inputStream, outputStream) {
-      @Override
-      public String process(String line) {
-        return transformLine(line);
-      }
-    };
-  }
-
   private static final ImmutableList<Pattern> PATH_PATTERNS =
       Platform.detect() == Platform.WINDOWS
           ? ImmutableList.of()

diff --git a/src/com/facebook/buck/cxx/CxxPreprocessAndCompileStep.java b/src/com/facebook/buck/cxx/CxxPreprocessAndCompileStep.java
@@ -26,16 +26,17 @@
 import com.facebook.buck.util.Console;
 import com.facebook.buck.util.DefaultProcessExecutor;
 import com.facebook.buck.util.Escaper;
-import com.facebook.buck.util.LineProcessorRunnable;
 import com.facebook.buck.util.MoreThrowables;
 import com.facebook.buck.util.ProcessExecutor;
 import com.facebook.buck.util.ProcessExecutorParams;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
-import java.io.ByteArrayOutputStream;
+import java.io.BufferedReader;
 import java.io.IOException;
+import java.io.InputStreamReader;
+import java.io.UncheckedIOException;
 import java.nio.file.Path;
 import java.util.HashMap;
 import java.util.Map;
@@ -188,28 +189,24 @@ private int executeCompilation(ExecutionContext context) throws Exception {
     ProcessExecutor.LaunchedProcess process = executor.launchProcess(params);
 
     // We buffer error messages in memory, as these are typically small.
-    ByteArrayOutputStream error = new ByteArrayOutputStream();
-
-    // Fire up managed threads to process the stdout and stderr lines.
+    String err;
     int exitCode;
-    try {
-      try (LineProcessorRunnable errorProcessor =
-          createErrorTransformerFactory(context)
-              .createTransformerThread(context, compiler.getErrorStream(process), error)) {
-        errorProcessor.start();
-        errorProcessor.waitFor();
-      } catch (Throwable thrown) {
-        executor.destroyLaunchedProcess(process);
-        throw thrown;
-      }
+    try (BufferedReader reader =
+        new BufferedReader(new InputStreamReader(compiler.getErrorStream(process)))) {
+      CxxErrorTransformer cxxErrorTransformer =
+          new CxxErrorTransformer(
+              filesystem, context.shouldReportAbsolutePaths(), headerPathNormalizer);
+      err =
+          reader.lines().map(cxxErrorTransformer::transformLine).collect(Collectors.joining("\n"));
       exitCode = executor.waitForLaunchedProcess(process).getExitCode();
+    } catch (UncheckedIOException e) {
+      throw e.getCause();
     } finally {
       executor.destroyLaunchedProcess(process);
       executor.waitForLaunchedProcess(process);
     }
 
     // If we generated any error output, print that to the console.
-    String err = new String(error.toByteArray());
     if (!err.isEmpty()) {
       context
           .getBuckEventBus()
@@ -233,11 +230,6 @@ private ConsoleEvent createConsoleEvent(
     }
   }
 
-  private CxxErrorTransformerFactory createErrorTransformerFactory(ExecutionContext context) {
-    return new CxxErrorTransformerFactory(
-        filesystem, context.shouldReportAbsolutePaths(), headerPathNormalizer);
-  }
-
   @Override
   public StepExecutionResult execute(ExecutionContext context) throws InterruptedException {
     try {

diff --git a/src/com/facebook/buck/util/BUCK b/src/com/facebook/buck/util/BUCK
@@ -34,7 +34,6 @@ IO_SRCS = [
     "DirectoryCleaner.java",
     "FilteredDirectoryCopier.java",
     "LineIterating.java",
-    "LineProcessorRunnable.java",
     "ListeningCharsetDecoder.java",
     "ListeningCharsetEncoder.java",
     "PkillProcessManager.java",

diff --git a/src/com/facebook/buck/util/LineProcessorRunnable.java b/src/com/facebook/buck/util/LineProcessorRunnable.java
diff --git a/src/com/facebook/buck/util/ManagedRunnable.java b/src/com/facebook/buck/util/ManagedRunnable.java
diff --git a/...k/cxx/CxxErrorTransformerFactoryTest.java → ...ook/buck/cxx/CxxErrorTransformerTest.java b/...k/cxx/CxxErrorTransformerFactoryTest.java → ...ook/buck/cxx/CxxErrorTransformerTest.java
@@ -42,7 +42,7 @@
  * absolute path modes.
  */
 @RunWith(Parameterized.class)
-public class CxxErrorTransformerFactoryTest {
+public class CxxErrorTransformerTest {
 
   @Parameterized.Parameters(name = "{index}: {0}")
   public static Collection<Object[]> data() {
@@ -63,13 +63,13 @@ public static Collection<Object[]> data() {
         new Object[][] {
           {
             "relative paths",
-            new CxxErrorTransformerFactory(filesystem, false, normalizer),
+            new CxxErrorTransformer(filesystem, false, normalizer),
             filesystem.relativize(replacement),
             original
           },
           {
             "absolute paths",
-            new CxxErrorTransformerFactory(filesystem, true, normalizer),
+            new CxxErrorTransformer(filesystem, true, normalizer),
             replacement.toAbsolutePath(),
             original
           }
@@ -80,7 +80,7 @@ public static Collection<Object[]> data() {
   public String datasetName;
 
   @Parameterized.Parameter(value = 1)
-  public CxxErrorTransformerFactory transformer;
+  public CxxErrorTransformer transformer;
 
   @Parameterized.Parameter(value = 2)
   public Path expectedPath;

diff --git a/windows_failures.txt b/windows_failures.txt
@@ -12,7 +12,7 @@
 !com.facebook.buck.cxx.CxxCompilationDatabaseTest
 !com.facebook.buck.cxx.CxxCompileStepIntegrationTest
 !com.facebook.buck.cxx.CxxDependencyFileIntegrationTest
-!com.facebook.buck.cxx.CxxErrorTransformerFactoryTest
+!com.facebook.buck.cxx.CxxErrorTransformerTest
 !com.facebook.buck.cxx.CxxGtestTestTest
 !com.facebook.buck.cxx.CxxLibraryDescriptionTest#createBuildRule
 !com.facebook.buck.cxx.CxxLibraryDescriptionTest#createBuildRule.*