8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time

lmesnik · lmesnik · commit 2e340e855b76 · 2023-04-26T14:53:33.000Z
Reviewed-by: cjplummer, sspitsyn
diff --git a/test/jdk/sun/tools/jstatd/JstatdTest.java b/test/jdk/sun/tools/jstatd/JstatdTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -350,7 +350,10 @@ private void runTest(boolean useShortSyntax) throws Throwable {
 
         // Verify output from jstatd
         OutputAnalyzer output = jstatdThread.getOutput();
-        output.shouldBeEmptyIgnoreVMWarnings();
+        List<String> stdout = output.asLinesWithoutVMWarnings();
+        output.reportDiagnosticSummary();
+        assertEquals(stdout.size(), 1, "Output should contain one line");
+        assertTrue(stdout.get(0).startsWith("jstatd started"), "List should start with 'jstatd started'");
         assertNotEquals(output.getExitValue(), 0,
                 "jstatd process exited with unexpected exit code");
     }
diff --git a/test/lib-test/jdk/test/lib/process/ProcessToolsStartProcessTest.java b/test/lib-test/jdk/test/lib/process/ProcessToolsStartProcessTest.java
@@ -0,0 +1,122 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @summary Unit test for ProcessTools.startProcess()
+ * @library /test/lib
+ * @compile ProcessToolsStartProcessTest.java
+ * @run main ProcessToolsStartProcessTest
+ */
+
+import java.util.function.Consumer;
+import java.io.File;
+
+import jdk.test.lib.JDKToolLauncher;
+import jdk.test.lib.Utils;
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+
+public class ProcessToolsStartProcessTest {
+    static final int NUM_LINES = 50;
+    static String output = "";
+
+    private static Consumer<String> outputConsumer = s -> {
+        output += s + "\n";
+    };
+
+    static boolean testStartProcess(boolean withConsumer) throws Exception {
+        boolean success = true;
+        Process p;
+        JDKToolLauncher launcher = JDKToolLauncher.createUsingTestJDK("java");
+        launcher.addToolArg("-cp");
+        launcher.addToolArg(Utils.TEST_CLASSES);
+        launcher.addToolArg("ProcessToolsStartProcessTest");
+        launcher.addToolArg("test"); // This one argument triggers producing the output
+        ProcessBuilder pb = new ProcessBuilder();
+        pb.command(launcher.getCommand());
+
+        System.out.println("DEBUG: Test with withConsumer=" + withConsumer);
+        System.out.println("DEBUG: about to start process.");
+        if (withConsumer) {
+            p = ProcessTools.startProcess("java", pb, outputConsumer);
+        } else {
+            p = ProcessTools.startProcess("java", pb);
+        }
+        OutputAnalyzer out = new OutputAnalyzer(p);
+
+        System.out.println("DEBUG: process started.");
+        p.waitFor();
+        if (p.exitValue() != 0) {
+            throw new RuntimeException("Bad exit value: " + p.exitValue());
+        }
+
+        if (withConsumer) {
+            int numLines = output.split("\n").length;
+            if (numLines != NUM_LINES ) {
+                System.out.print("FAILED: wrong number of lines in Consumer output\n");
+                success = false;
+            }
+            System.out.println("DEBUG: Consumer output: got " + numLines + " lines , expected "
+                               + NUM_LINES  + ". Output follow:");
+            System.out.print(output);
+            System.out.println("DEBUG: done with Consumer output.");
+        }
+
+        int numLinesOut = out.getStdout().split("\n").length;
+        if (numLinesOut != NUM_LINES) {
+            System.out.print("FAILED: wrong number of lines in OutputAnalyzer output\n");
+            success = false;
+        }
+        System.out.println("DEBUG: OutputAnalyzer output: got " + numLinesOut + " lines, expected "
+                           + NUM_LINES  + ". Output follows:");
+        System.out.print(out.getStdout());
+        System.out.println("DEBUG: done with OutputAnalyzer stdout.");
+
+        return success;
+    }
+
+    public static void main(String[] args) {
+        if (args.length > 0) {
+            for (int i = 0; i < NUM_LINES; i++) {
+                System.out.println("A line on stdout " + i);
+            }
+        } else {
+            try {
+                boolean test1Result = testStartProcess(false);
+                boolean test2Result = testStartProcess(true);
+                if (!test1Result || !test2Result) {
+                    throw new RuntimeException("One or more tests failed. See output for details.");
+                }
+            } catch (RuntimeException re) {
+                re.printStackTrace();
+                System.out.println("Test ERROR");
+                throw re;
+            } catch (Exception ex) {
+                ex.printStackTrace();
+                System.out.println("Test ERROR");
+                throw new RuntimeException(ex);
+            }
+        }
+    }
+}
diff --git a/test/lib/jdk/test/lib/process/ProcessTools.java b/test/lib/jdk/test/lib/process/ProcessTools.java
@@ -27,6 +27,7 @@
 import jdk.test.lib.Platform;
 import jdk.test.lib.Utils;
 
+import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
@@ -71,14 +72,17 @@ protected void processLine(String line) {
             ps.println("[" + prefix + "] " + line);
         }
     }
-
     private ProcessTools() {
     }
 
     /**
      * <p>Starts a process from its builder.</p>
      * <span>The default redirects of STDOUT and STDERR are started</span>
-     *
+     * <p>
+     * Same as
+     * {@linkplain #startProcess(String, ProcessBuilder, Consumer, Predicate, long, TimeUnit) startProcess}
+     * {@code (name, processBuilder, null, null, -1, TimeUnit.NANOSECONDS)}
+     * </p>
      * @param name           The process name
      * @param processBuilder The process builder
      * @return Returns the initialized process
@@ -93,11 +97,15 @@ public static Process startProcess(String name,
     /**
      * <p>Starts a process from its builder.</p>
      * <span>The default redirects of STDOUT and STDERR are started</span>
-     * <p>It is possible to monitor the in-streams via the provided {@code consumer}
+     * <p>
+     * Same as
+     * {@linkplain #startProcess(String, ProcessBuilder, Consumer, Predicate, long, TimeUnit) startProcess}
+     * {@code (name, processBuilder, consumer, null, -1, TimeUnit.NANOSECONDS)}
+     * </p>
      *
      * @param name           The process name
-     * @param consumer       {@linkplain Consumer} instance to process the in-streams
      * @param processBuilder The process builder
+     * @param consumer       {@linkplain Consumer} instance to process the in-streams
      * @return Returns the initialized process
      * @throws IOException
      */
@@ -118,8 +126,9 @@ public static Process startProcess(String name,
      * <p>Starts a process from its builder.</p>
      * <span>The default redirects of STDOUT and STDERR are started</span>
      * <p>
-     * It is possible to wait for the process to get to a warmed-up state
-     * via {@linkplain Predicate} condition on the STDOUT/STDERR
+     * Same as
+     * {@linkplain #startProcess(String, ProcessBuilder, Consumer, Predicate, long, TimeUnit) startProcess}
+     * {@code (name, processBuilder, null, linePredicate, timeout, unit)}
      * </p>
      *
      * @param name           The process name
@@ -144,6 +153,58 @@ public static Process startProcess(String name,
         return startProcess(name, processBuilder, null, linePredicate, timeout, unit);
     }
 
+
+    /*
+        BufferOutputStream and BufferInputStream allow to re-use p.getInputStream() amd p.getOutputStream() of
+        processes started with ProcessTools.startProcess(...).
+        Implementation cashes ALL process output and allow to read it through InputStream.
+     */
+    private static class BufferOutputStream extends ByteArrayOutputStream {
+        private int current = 0;
+        final private Process p;
+
+        public BufferOutputStream(Process p) {
+            this.p = p;
+        }
+
+        synchronized int readNext() {
+            if (current > count) {
+                throw new RuntimeException("Shouldn't ever happen.  start: "
+                        + current + " count: " + count + " buffer: " + this);
+            }
+            while (current == count) {
+                if (!p.isAlive()) {
+                    return -1;
+                }
+                try {
+                    wait(1);
+                } catch (InterruptedException ie) {
+                    return -1;
+                }
+            }
+            return this.buf[current++];
+        }
+    }
+
+    private static class BufferInputStream extends InputStream {
+
+        private final BufferOutputStream buffer;
+
+        public BufferInputStream(Process p) {
+            buffer = new BufferOutputStream(p);
+        }
+
+        OutputStream getOutputStream() {
+            return buffer;
+        }
+
+        @Override
+        public int read() throws IOException {
+            return buffer.readNext();
+        }
+    }
+
+
     /**
      * <p>Starts a process from its builder.</p>
      * <span>The default redirects of STDOUT and STDERR are started</span>
@@ -181,6 +242,12 @@ public static Process startProcess(String name,
 
         stdout.addPump(new LineForwarder(name, System.out));
         stderr.addPump(new LineForwarder(name, System.err));
+        BufferInputStream stdOut = new BufferInputStream(p);
+        BufferInputStream stdErr = new BufferInputStream(p);
+
+        stdout.addOutputStream(stdOut.getOutputStream());
+        stderr.addOutputStream(stdErr.getOutputStream());
+
         if (lineConsumer != null) {
             StreamPumper.LinePump pump = new StreamPumper.LinePump() {
                 @Override
@@ -250,7 +317,7 @@ protected void processLine(String line) {
             throw e;
         }
 
-        return new ProcessImpl(p, stdoutTask, stderrTask);
+        return new ProcessImpl(p, stdoutTask, stderrTask, stdOut, stdErr);
     }
 
     /**
@@ -701,14 +768,19 @@ private static Process privilegedStart(ProcessBuilder pb) throws IOException {
 
     private static class ProcessImpl extends Process {
 
+        private final InputStream stdOut;
+        private final InputStream stdErr;
         private final Process p;
         private final Future<Void> stdoutTask;
         private final Future<Void> stderrTask;
 
-        public ProcessImpl(Process p, Future<Void> stdoutTask, Future<Void> stderrTask) {
+        public ProcessImpl(Process p, Future<Void> stdoutTask, Future<Void> stderrTask,
+                           InputStream stdOut, InputStream etdErr) {
             this.p = p;
             this.stdoutTask = stdoutTask;
             this.stderrTask = stderrTask;
+            this.stdOut = stdOut;
+            this.stdErr = etdErr;
         }
 
         @Override
@@ -718,12 +790,12 @@ public OutputStream getOutputStream() {
 
         @Override
         public InputStream getInputStream() {
-            return p.getInputStream();
+            return stdOut;
         }
 
         @Override
         public InputStream getErrorStream() {
-            return p.getErrorStream();
+            return stdErr;
         }
 
         @Override
