[JENKINS-52729] Launcher.ProcStarter.stdout(TaskListener) discards remotability (#3563)

* [JENKINS-52729] Demonstrating issue with Launcher.ProcStarter.stdout(TaskListener).

* [JENKINS-52729] Properly remote the TaskListener.

* Defend test against a possible problem on Windows.

* FindBugs

* Fix a Windows test failure. There is no C:\Windows\System32\echo.exe; it is a cmd.exe built-in.

* Clarify in Javadoc that TaskListener is remotable.

* A call to stdout(TaskListener) should be countermanded by a call to stdout(OutputStream), which was true for local but not remote launchers.

Author: jglick

core/src/main/java/hudson/Launcher.java

@@ -166,6 +166,8 @@ public final class ProcStarter {
         @CheckForNull
         protected OutputStream stdout = NULL_OUTPUT_STREAM, stderr;
         @CheckForNull
+        private TaskListener stdoutListener;
+        @CheckForNull
         protected InputStream stdin = NULL_INPUT_STREAM;
         @CheckForNull
         protected String[] envs = null;
@@ -285,17 +287,20 @@ public FilePath pwd() {
          */
         public ProcStarter stdout(@CheckForNull OutputStream out) {
             this.stdout = out;
+            stdoutListener = null;
             return this;
         }
 
         /**
          * Sends the stdout to the given {@link TaskListener}.
          * 
-         * @param out Task listener
+         * @param out Task listener (must be safely remotable)
          * @return {@code this}
          */
         public ProcStarter stdout(@Nonnull TaskListener out) {
-            return stdout(out.getLogger());
+            stdout = out.getLogger();
+            stdoutListener = out;
+            return this;
         }
 
         /**
@@ -490,6 +495,7 @@ public int join() throws IOException, InterruptedException {
         @Nonnull
         public ProcStarter copy() {
             ProcStarter rhs = new ProcStarter().cmds(commands).pwd(pwd).masks(masks).stdin(stdin).stdout(stdout).stderr(stderr).envs(envs).quiet(quiet);
+            rhs.stdoutListener = stdoutListener;
             rhs.reverseStdin  = this.reverseStdin;
             rhs.reverseStderr = this.reverseStderr;
             rhs.reverseStdout = this.reverseStdout;
@@ -1041,15 +1047,15 @@ public VirtualChannel getChannel() {
         }
 
         public Proc launch(ProcStarter ps) throws IOException {
-            final OutputStream out = ps.stdout == null ? null : new RemoteOutputStream(new CloseProofOutputStream(ps.stdout));
+            final OutputStream out = ps.stdout == null || ps.stdoutListener != null ? null : new RemoteOutputStream(new CloseProofOutputStream(ps.stdout));
             final OutputStream err = ps.stderr==null ? null : new RemoteOutputStream(new CloseProofOutputStream(ps.stderr));
             final InputStream  in  = (ps.stdin==null || ps.stdin==NULL_INPUT_STREAM) ? null : new RemoteInputStream(ps.stdin,false);
 
             final FilePath psPwd = ps.pwd;
             final String workDir = psPwd==null ? null : psPwd.getRemote();
 
             try {
-                return new ProcImpl(getChannel().call(new RemoteLaunchCallable(ps.commands, ps.masks, ps.envs, in, ps.reverseStdin, out, ps.reverseStdout, err, ps.reverseStderr, ps.quiet, workDir, listener)));
+                return new ProcImpl(getChannel().call(new RemoteLaunchCallable(ps.commands, ps.masks, ps.envs, in, ps.reverseStdin, out, ps.reverseStdout, err, ps.reverseStderr, ps.quiet, workDir, listener, ps.stdoutListener)));
             } catch (InterruptedException e) {
                 throw (IOException)new InterruptedIOException().initCause(e);
             }
@@ -1265,14 +1271,15 @@ private static class RemoteLaunchCallable extends MasterToSlaveCallable<RemotePr
         private final @CheckForNull OutputStream err;
         private final @CheckForNull String workDir;
         private final @Nonnull TaskListener listener;
+        private final @CheckForNull TaskListener stdoutListener;
         private final boolean reverseStdin, reverseStdout, reverseStderr;
         private final boolean quiet;
 
         RemoteLaunchCallable(@Nonnull List<String> cmd, @CheckForNull boolean[] masks, @CheckForNull String[] env, 
                 @CheckForNull InputStream in, boolean reverseStdin, 
                 @CheckForNull OutputStream out, boolean reverseStdout, 
                 @CheckForNull OutputStream err, boolean reverseStderr, 
-                boolean quiet, @CheckForNull String workDir, @Nonnull TaskListener listener) {
+                boolean quiet, @CheckForNull String workDir, @Nonnull TaskListener listener, @CheckForNull TaskListener stdoutListener) {
             this.cmd = new ArrayList<>(cmd);
             this.masks = masks;
             this.env = env;
@@ -1281,6 +1288,7 @@ private static class RemoteLaunchCallable extends MasterToSlaveCallable<RemotePr
             this.err = err;
             this.workDir = workDir;
             this.listener = listener;
+            this.stdoutListener = stdoutListener;
             this.reverseStdin = reverseStdin;
             this.reverseStdout = reverseStdout;
             this.reverseStderr = reverseStderr;
@@ -1290,7 +1298,12 @@ private static class RemoteLaunchCallable extends MasterToSlaveCallable<RemotePr
         public RemoteProcess call() throws IOException {
             final Channel channel = getOpenChannelOrFail();
             Launcher.ProcStarter ps = new LocalLauncher(listener).launch();
-            ps.cmds(cmd).masks(masks).envs(env).stdin(in).stdout(out).stderr(err).quiet(quiet);
+            ps.cmds(cmd).masks(masks).envs(env).stdin(in).stderr(err).quiet(quiet);
+            if (stdoutListener != null) {
+                ps.stdout(stdoutListener.getLogger());
+            } else {
+                ps.stdout(out);
+            }
             if(workDir!=null)   ps.pwd(workDir);
             if (reverseStdin)   ps.writeStdin();
             if (reverseStdout)  ps.readStdout();

core/src/main/java/hudson/model/TaskListener.java

@@ -25,6 +25,7 @@
 
 import hudson.console.ConsoleNote;
 import hudson.console.HyperlinkNote;
+import hudson.remoting.Channel;
 import hudson.util.NullStream;
 import hudson.util.StreamTaskListener;
 
@@ -59,7 +60,9 @@
  *
  * <p>
  * {@link StreamTaskListener} is the most typical implementation of this interface.
- * All the {@link TaskListener} implementations passed to plugins from Hudson core are remotable.
+ *
+ * <p>
+ * Implementations are generally expected to be remotable via {@link Channel}.
  *
  * @author Kohsuke Kawaguchi
  */

test/src/test/java/hudson/LauncherTest.java

@@ -23,24 +23,38 @@
  */
 package hudson;
 
+import hudson.console.LineTransformationOutputStream;
 import hudson.model.AbstractBuild;
 import hudson.model.AbstractProject;
+import hudson.model.BuildListener;
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
 import hudson.model.Node;
 import hudson.model.ParametersDefinitionProperty;
 import hudson.model.Slave;
 import hudson.model.StringParameterDefinition;
 import hudson.model.TaskListener;
+import hudson.remoting.Channel;
 import hudson.tasks.BatchFile;
 import hudson.tasks.BuildStepDescriptor;
 import hudson.tasks.Builder;
 import hudson.tasks.CommandInterpreter;
 import hudson.tasks.Shell;
+import hudson.util.StreamTaskListener;
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
 import java.io.IOException;
+import java.io.OutputStream;
+import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
 
 import java.util.HashMap;
 import java.util.Map;
+import org.apache.commons.io.FileUtils;
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+import static org.junit.Assume.*;
 
 import org.junit.Rule;
 import org.junit.Test;
@@ -153,4 +167,110 @@ private static final class QuietBatchFile extends BatchFile {
         }
     }
 
+    @Issue("JENKINS-52729")
+    @Test public void remotable() throws Exception {
+        File log = new File(rule.jenkins.root, "log");
+        TaskListener listener = new RemotableBuildListener(log);
+        Launcher.ProcStarter ps = rule.createOnlineSlave().createLauncher(listener).launch();
+        if (Functions.isWindows()) {
+            ps.cmds("cmd", "/c", "echo", "hello");
+        } else {
+            ps.cmds("echo", "hello");
+        }
+        assertEquals(0, ps.stdout(listener).join());
+        assertThat(FileUtils.readFileToString(log, StandardCharsets.UTF_8).replace("\r\n", "\n"),
+            containsString("[master → slave0] $ " + (Functions.isWindows() ? "cmd /c " : "") + "echo hello\n" +
+                           "[master → slave0] hello"));
+    }
+    private static class RemotableBuildListener implements BuildListener {
+        private static final long serialVersionUID = 1;
+        /** location of log file streamed to by multiple sources */
+        private final File logFile;
+        /** records allocation & deserialization history; e.g., {@code master → agentName} */
+        private final String id;
+        private transient PrintStream logger;
+        RemotableBuildListener(File logFile) {
+            this(logFile, "master");
+        }
+        private RemotableBuildListener(File logFile, String id) {
+            this.logFile = logFile;
+            this.id = id;
+        }
+        @Override public PrintStream getLogger() {
+            if (logger == null) {
+                final OutputStream fos;
+                try {
+                    fos = new FileOutputStream(logFile, true);
+                    logger = new PrintStream(new LineTransformationOutputStream() {
+                        @Override protected void eol(byte[] b, int len) throws IOException {
+                            fos.write(("[" + id + "] ").getBytes(StandardCharsets.UTF_8));
+                            fos.write(b, 0, len);
+                        }
+                    }, true, "UTF-8");
+                } catch (IOException x) {
+                    throw new AssertionError(x);
+                }
+            }
+            return logger;
+        }
+        private Object writeReplace() {
+            Thread.dumpStack();
+            String name = Channel.current().getName();
+            return new RemotableBuildListener(logFile, id + " → " + name);
+        }
+    }
+
+    @Issue("JENKINS-52729")
+    @Test public void multipleStdioCalls() throws Exception {
+        Node master = rule.jenkins;
+        Node agent = rule.createOnlineSlave();
+        for (Node node : new Node[] {master, agent}) {
+            assertMultipleStdioCalls("first TaskListener then OutputStream", node, false, (ps, os1, os2, os2Listener) -> {
+                ps.stdout(os2Listener).stdout(os1);
+                assertEquals(os1, ps.stdout());
+            }, false);
+            assertMultipleStdioCalls("first OutputStream then TaskListener", node, false, (ps, os1, os2, os2Listener) -> {
+                ps.stdout(os1).stdout(os2Listener);
+                assertEquals(os2Listener.getLogger(), ps.stdout());
+            }, true);
+            assertMultipleStdioCalls("stdout then stderr", node, true, (ps, os1, os2, os2Listener) -> {
+                ps.stdout(os1).stderr(os2);
+                assertEquals(os1, ps.stdout());
+                assertEquals(os2, ps.stderr());
+            }, true);
+            assertMultipleStdioCalls("stderr then stdout", node, true, (ps, os1, os2, os2Listener) -> {
+                ps.stdout(os1).stderr(os2);
+                assertEquals(os1, ps.stdout());
+                assertEquals(os2, ps.stderr());
+            }, true);
+        }
+    }
+    @FunctionalInterface
+    private interface ProcStarterCustomizer {
+        void run(Launcher.ProcStarter ps, OutputStream os1, OutputStream os2, TaskListener os2Listener) throws Exception;
+    }
+    private void assertMultipleStdioCalls(String message, Node node, boolean emitStderr, ProcStarterCustomizer psCustomizer, boolean outputIn2) throws Exception {
+        message = node.getDisplayName() + ": " + message;
+        Launcher launcher = node.createLauncher(StreamTaskListener.fromStderr());
+        Launcher.ProcStarter ps = launcher.launch();
+        assumeFalse("should not be platform-dependent, not bothering for now", Functions.isWindows());
+        if (emitStderr) {
+            ps.cmds("sh", "-c", "echo hello >&2").quiet(true);
+        } else {
+            ps.cmds("echo", "hello");
+        }
+        ByteArrayOutputStream baos1 = new ByteArrayOutputStream();
+        ByteArrayOutputStream baos2 = new ByteArrayOutputStream();
+        TaskListener listener = new StreamTaskListener(baos2);
+        psCustomizer.run(ps, baos1, baos2, listener);
+        assertEquals(message, 0, ps.join());
+        if (outputIn2) {
+            assertThat(message, baos2.toString(), containsString("hello"));
+            assertThat(message, baos1.toString(), isEmptyString());
+        } else {
+            assertThat(message, baos1.toString(), containsString("hello"));
+            assertThat(message, baos2.toString(), isEmptyString());
+        }
+    }
+
 }