[JEP-206] Define API for gathering command output in a local encoding

src/main/java/org/jenkinsci/plugins/durabletask/Controller.java

@@ -49,6 +49,8 @@ public abstract class Controller implements Serializable {
      * @param workspace the workspace in use
      * @param sink where to send new log output
      * @return true if something was written and the controller should be resaved, false if everything is idle
+     * @see DurableTask#charset
+     * @see DurableTask#defaultCharset
      */
     public abstract boolean writeLog(FilePath workspace, OutputStream sink) throws IOException, InterruptedException;
 
@@ -88,6 +90,8 @@ public abstract class Controller implements Serializable {
      * @param workspace the workspace in use
      * @param launcher a way to start processes
      * @return the output of the process as raw bytes (may be empty but not null)
+     * @see DurableTask#charset
+     * @see DurableTask#defaultCharset
      */
     public @Nonnull byte[] getOutput(@Nonnull FilePath workspace, @Nonnull Launcher launcher) throws IOException, InterruptedException {
         throw new IOException("Did not implement getOutput in " + getClass().getName());

src/main/java/org/jenkinsci/plugins/durabletask/DurableTask.java

@@ -31,6 +31,8 @@
 import hudson.model.AbstractDescribableImpl;
 import hudson.model.TaskListener;
 import java.io.IOException;
+import java.nio.charset.Charset;
+import javax.annotation.Nonnull;
 
 /**
  * A task which may be run asynchronously on a build node and withstand disconnection of the slave agent.
@@ -63,4 +65,23 @@ public void captureOutput() throws UnsupportedOperationException {
         throw new UnsupportedOperationException("Capturing of output is not implemented in " + getClass().getName());
     }
 
+    /**
+     * Requests that a specified charset be used to transcode process output.
+     * The encoding of {@link Controller#writeLog} and {@link Controller#getOutput} is then presumed to be UTF-8.
+     * If not called, no translation is performed.
+     * @param cs the character set in which process output is expected to be
+     */
+    public void charset(@Nonnull Charset cs) {
+        // by default, ignore
+    }
+
+    /**
+     * Requests that the node’s system charset be used to transcode process output.
+     * The encoding of {@link Controller#writeLog} and {@link Controller#getOutput} is then presumed to be UTF-8.
+     * If not called, no translation is performed.
+     */
+    public void defaultCharset() {
+        // by default, ignore
+    }
+
 }

src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java

@@ -37,22 +37,25 @@
 import hudson.util.StreamTaskListener;
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.RandomAccessFile;
 import java.io.StringWriter;
+import java.nio.ByteBuffer;
+import java.nio.channels.Channels;
 import java.nio.charset.Charset;
+import java.nio.charset.CodingErrorAction;
+import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.UUID;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 import jenkins.MasterToSlaveFileCallable;
+import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.IOUtils;
-import org.jenkinsci.remoting.RoleChecker;
-
-import javax.annotation.CheckForNull;
 
 /**
  * A task which forks some external command and then waits for log and status files to be updated/created.
@@ -63,19 +66,37 @@ public abstract class FileMonitoringTask extends DurableTask {
 
     private static final String COOKIE = "JENKINS_SERVER_COOKIE";
 
+    /** Value of {@link #charset} used to mean the node’s system default. */
+    private static final String SYSTEM_DEFAULT_CHARSET = "SYSTEM_DEFAULT";
+
+    /**
+     * Charset name to use for transcoding, or {@link #SYSTEM_DEFAULT_CHARSET}, or null for no transcoding.
+     */
+    private @CheckForNull String charset;
+
     private static String cookieFor(FilePath workspace) {
         return "durable-" + Util.getDigestOf(workspace.getRemote());
     }
 
     @Override public final Controller launch(EnvVars env, FilePath workspace, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
-        return launchWithCookie(workspace, launcher, listener, env, COOKIE, cookieFor(workspace));
+        FileMonitoringController controller = launchWithCookie(workspace, launcher, listener, env, COOKIE, cookieFor(workspace));
+        controller.charset = charset;
+        return controller;
     }
 
     protected FileMonitoringController launchWithCookie(FilePath workspace, Launcher launcher, TaskListener listener, EnvVars envVars, String cookieVariable, String cookieValue) throws IOException, InterruptedException {
         envVars.put(cookieVariable, cookieValue); // ensure getCharacteristicEnvVars does not match, so Launcher.killAll will leave it alone
         return doLaunch(workspace, launcher, listener, envVars);
     }
 
+    @Override public final void charset(Charset cs) {
+        charset = cs.name();
+    }
+
+    @Override public final void defaultCharset() {
+        charset = SYSTEM_DEFAULT_CHARSET;
+    }
+
     /**
      * Should start a process which sends output to {@linkplain FileMonitoringController#getLogFile(FilePath) log file}
      * in the workspace and finally writes its exit code to {@linkplain FileMonitoringController#getResultFile(FilePath) result file}.
@@ -115,6 +136,9 @@ protected static class FileMonitoringController extends Controller {
          */
         private long lastLocation;
 
+        /** @see FileMonitoringTask#charset */
+        private @CheckForNull String charset;
+
         protected FileMonitoringController(FilePath ws) throws IOException, InterruptedException {
             // can't keep ws reference because Controller is expected to be serializable
             ws.mkdirs();
@@ -125,7 +149,7 @@ protected FileMonitoringController(FilePath ws) throws IOException, InterruptedE
 
         @Override public final boolean writeLog(FilePath workspace, OutputStream sink) throws IOException, InterruptedException {
             FilePath log = getLogFile(workspace);
-            Long newLocation = log.act(new WriteLog(lastLocation, new RemoteOutputStream(sink)));
+            Long newLocation = log.act(new WriteLog(lastLocation, new RemoteOutputStream(sink), charset));
             if (newLocation != null) {
                 LOGGER.log(Level.FINE, "copied {0} bytes from {1}", new Object[] {newLocation - lastLocation, log});
                 lastLocation = newLocation;
@@ -137,9 +161,11 @@ protected FileMonitoringController(FilePath ws) throws IOException, InterruptedE
         private static class WriteLog extends MasterToSlaveFileCallable<Long> {
             private final long lastLocation;
             private final OutputStream sink;
-            WriteLog(long lastLocation, OutputStream sink) {
+            private final @CheckForNull String charset;
+            WriteLog(long lastLocation, OutputStream sink, String charset) {
                 this.lastLocation = lastLocation;
                 this.sink = sink;
+                this.charset = charset;
             }
             @Override public Long invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
                 long len = f.length();
@@ -154,7 +180,12 @@ private static class WriteLog extends MasterToSlaveFileCallable<Long> {
                         // TODO is this efficient for large amounts of output? Would it be better to stream data, or return a byte[] from the callable?
                         byte[] buf = new byte[(int) toRead];
                         raf.readFully(buf);
-                        sink.write(buf);
+                        ByteBuffer transcoded = maybeTranscode(buf, charset);
+                        if (transcoded == null) {
+                            sink.write(buf);
+                        } else {
+                            Channels.newChannel(sink).write(transcoded);
+                        }
                     } finally {
                         raf.close();
                     }
@@ -198,13 +229,42 @@ public Integer invoke(File f, VirtualChannel channel) throws IOException, Interr
             return status.act(STATUS_CHECK_INSTANCE);
         }
 
-        @Override public byte[] getOutput(FilePath workspace, Launcher launcher) throws IOException, InterruptedException {
-            // TODO could perhaps be more efficient for large files to send a MasterToSlaveFileCallable<byte[]>
-            try (InputStream is = getOutputFile(workspace).read()) {
-                return IOUtils.toByteArray(is);
+        /**
+         * Transcode process output to UTF-8 if necessary.
+         * @param data output presumed to be in local encoding
+         * @param charset a particular encoding name, or the empty string for the system default encoding, or null to skip transcoding
+         * @return a buffer of UTF-8 encoded data ({@link CodingErrorAction#REPLACE} is used),
+         *         or null if not performing transcoding because it was not requested or the data was already thought to be in UTF-8
+         */
+        private static @CheckForNull ByteBuffer maybeTranscode(@Nonnull byte[] data, @CheckForNull String charset) {
+            if (charset == null) { // no transcoding requested, do raw copy and YMMV
+                return null;
+            } else {
+                Charset cs = charset.equals(SYSTEM_DEFAULT_CHARSET) ? Charset.defaultCharset() : Charset.forName(charset);
+                if (cs.equals(StandardCharsets.UTF_8)) { // transcoding unnecessary as output was already UTF-8
+                    return null;
+                } else { // decode output in specified charset and reëncode in UTF-8
+                    return StandardCharsets.UTF_8.encode(cs.decode(ByteBuffer.wrap(data)));
+                }
             }
         }
 
+        @Override public byte[] getOutput(FilePath workspace, Launcher launcher) throws IOException, InterruptedException {
+            return getOutputFile(workspace).act(new MasterToSlaveFileCallable<byte[]>() {
+                @Override public byte[] invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+                    byte[] buf = FileUtils.readFileToByteArray(f);
+                    ByteBuffer transcoded = maybeTranscode(buf, charset);
+                    if (transcoded == null) {
+                        return buf;
+                    } else {
+                        byte[] buf2 = new byte[transcoded.remaining()];
+                        transcoded.get(buf2);
+                        return buf2;
+                    }
+                }
+            });
+        }
+
         @Override public final void stop(FilePath workspace, Launcher launcher) throws IOException, InterruptedException {
             launcher.kill(Collections.singletonMap(COOKIE, cookieFor(workspace)));
         }

src/test/java/org/jenkinsci/plugins/durabletask/BourneShellScriptTest.java

@@ -34,8 +34,11 @@
 import hudson.util.VersionNumber;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.logging.Level;
+import jenkins.security.MasterToSlaveCallable;
 import org.apache.commons.io.output.TeeOutputStream;
 import static org.hamcrest.Matchers.*;
 import org.jenkinsci.test.acceptance.docker.Docker;
@@ -236,4 +239,141 @@ private void runOnDocker(DumbSlave s) throws Exception {
         runOnDocker(new DumbSlave("docker", "/home/jenkins/agent", new SimpleCommandLauncher("docker run -i --rm --name agent --init jenkinsci/slave:3.7-1 java -jar /usr/share/jenkins/slave.jar")));
     }
 
+    @Issue("JENKINS-31096")
+    @Test public void encoding() throws Exception {
+        JavaContainer container = dockerUbuntu.get();
+        DumbSlave s = new DumbSlave("docker", "/home/test", new SSHLauncher(container.ipBound(22), container.port(22), "test", "test", "", "-Dfile.encoding=ISO-8859-1"));
+        j.jenkins.addNode(s);
+        j.waitOnline(s);
+        assertEquals("ISO-8859-1", s.getChannel().call(new DetectCharset()));
+        FilePath dockerWS = s.getWorkspaceRoot();
+        dockerWS.child("latin").write("¡Ole!", "ISO-8859-1");
+        dockerWS.child("eastern").write("Čau!", "ISO-8859-2");
+        dockerWS.child("mixed").write("¡Čau → there!", "UTF-8");
+        Launcher dockerLauncher = s.createLauncher(listener);
+        // control: no transcoding
+        Controller c = new BourneShellScript("cat latin").launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        c.writeLog(dockerWS, baos);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertThat(baos.toString("ISO-8859-1"), containsString("¡Ole!"));
+        c.cleanup(dockerWS);
+        // and with output capture:
+        BourneShellScript dt = new BourneShellScript("cat latin");
+        dt.captureOutput();
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        c.writeLog(dockerWS, System.err);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertEquals("¡Ole!", new String(c.getOutput(dockerWS, launcher), "ISO-8859-1"));
+        c.cleanup(dockerWS);
+        // test: specify particular charset (UTF-8)
+        dt = new BourneShellScript("cat mixed");
+        dt.charset(StandardCharsets.UTF_8);
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        baos = new ByteArrayOutputStream();
+        c.writeLog(dockerWS, baos);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertThat(baos.toString("UTF-8"), containsString("¡Čau → there!"));
+        c.cleanup(dockerWS);
+        // and with output capture:
+        dt = new BourneShellScript("cat mixed");
+        dt.charset(StandardCharsets.UTF_8);
+        dt.captureOutput();
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        c.writeLog(dockerWS, System.err);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertEquals("¡Čau → there!", new String(c.getOutput(dockerWS, launcher), "UTF-8"));
+        c.cleanup(dockerWS);
+        // test: specify particular charset (unrelated)
+        dt = new BourneShellScript("cat eastern");
+        dt.charset(Charset.forName("ISO-8859-2"));
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        baos = new ByteArrayOutputStream();
+        c.writeLog(dockerWS, baos);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertThat(baos.toString("UTF-8"), containsString("Čau!"));
+        c.cleanup(dockerWS);
+        // and with output capture:
+        dt = new BourneShellScript("cat eastern");
+        dt.charset(Charset.forName("ISO-8859-2"));
+        dt.captureOutput();
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        c.writeLog(dockerWS, System.err);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertEquals("Čau!", new String(c.getOutput(dockerWS, launcher), "UTF-8"));
+        c.cleanup(dockerWS);
+        // test: specify agent default charset
+        dt = new BourneShellScript("cat latin");
+        dt.defaultCharset();
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        baos = new ByteArrayOutputStream();
+        c.writeLog(dockerWS, baos);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertThat(baos.toString("UTF-8"), containsString("¡Ole!"));
+        c.cleanup(dockerWS);
+        // and with output capture:
+        dt = new BourneShellScript("cat latin");
+        dt.defaultCharset();
+        dt.captureOutput();
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        c.writeLog(dockerWS, System.err);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertEquals("¡Ole!", new String(c.getOutput(dockerWS, launcher), "UTF-8"));
+        c.cleanup(dockerWS);
+        // test: inappropriate charset, some replacement characters
+        dt = new BourneShellScript("cat mixed");
+        dt.charset(StandardCharsets.US_ASCII);
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        baos = new ByteArrayOutputStream();
+        c.writeLog(dockerWS, baos);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertThat(baos.toString("UTF-8"), containsString("����au ��� there!"));
+        c.cleanup(dockerWS);
+        // and with output capture:
+        dt = new BourneShellScript("cat mixed");
+        dt.charset(StandardCharsets.US_ASCII);
+        dt.captureOutput();
+        c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            Thread.sleep(100);
+        }
+        c.writeLog(dockerWS, System.err);
+        assertEquals(0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        assertEquals("����au ��� there!", new String(c.getOutput(dockerWS, launcher), "UTF-8"));
+        c.cleanup(dockerWS);
+        s.toComputer().disconnect(new OfflineCause.UserCause(null, null));
+    }
+    private static class DetectCharset extends MasterToSlaveCallable<String, RuntimeException> {
+        @Override public String call() throws RuntimeException {
+            return Charset.defaultCharset().name();
+        }
+    }
+
 }