Don't buffer logs in memory when viewing log as plain text (#706)

Author: timja

pom.xml

@@ -143,18 +143,4 @@
       <url>https://repo.jenkins-ci.org/public/</url>
     </pluginRepository>
   </pluginRepositories>
-
-  <build>
-    <plugins>
-      <plugin>
-        <groupId>org.jenkins-ci.tools</groupId>
-        <artifactId>maven-hpi-plugin</artifactId>
-        <configuration>
-          <loggers>
-            <io.jenkins.plugins.pipelinegraphview>FINEST</io.jenkins.plugins.pipelinegraphview>
-          </loggers>
-        </configuration>
-      </plugin>
-    </plugins>
-  </build>
 </project>

src/main/java/io/jenkins/plugins/pipelinegraphview/consoleview/PipelineConsoleViewAction.java

@@ -16,7 +16,6 @@
 import io.jenkins.plugins.pipelinegraphview.utils.PipelineStepApi;
 import io.jenkins.plugins.pipelinegraphview.utils.PipelineStepList;
 import java.io.IOException;
-import java.io.Writer;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -28,8 +27,6 @@
 import org.kohsuke.stapler.StaplerRequest2;
 import org.kohsuke.stapler.StaplerResponse2;
 import org.kohsuke.stapler.WebMethod;
-import org.kohsuke.stapler.framework.io.CharSpool;
-import org.kohsuke.stapler.framework.io.LineEndNormalizingWriter;
 import org.kohsuke.stapler.verb.GET;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -120,45 +117,42 @@ protected JSONObject getAllSteps() throws IOException {
     }
 
     @WebMethod(name = "log")
-    public HttpResponse getConsoleText(StaplerRequest2 req, StaplerResponse2 rsp) throws IOException {
+    public void getConsoleText(StaplerRequest2 req, StaplerResponse2 rsp) throws IOException {
         String nodeId = req.getParameter("nodeId");
         if (nodeId == null) {
             logger.error("'consoleText' was not passed 'nodeId'.");
-            return HttpResponses.errorJSON("Error getting console text");
+            rsp.getWriter().write("Error getting console text\n");
+            return;
         }
         logger.debug("getConsoleText was passed node id '{}'.", nodeId);
         // This will be a step, so return its log output.
         AnnotatedLargeText<? extends FlowNode> logText = getLogForNode(nodeId);
+        if (logText != null) {
+            logText.writeLogTo(0L, rsp.getOutputStream());
+            return;
+        }
 
-        long count = 0;
+        // Potentially a stage, so get the log text for the stage.
+        boolean foundLogs = false;
         PipelineStepList steps = stepApi.getSteps(nodeId);
-        try (CharSpool spool = new CharSpool()) {
-
-            for (PipelineStep step : steps.getSteps()) {
-                AnnotatedLargeText<? extends FlowNode> logForNode = getLogForNode(String.valueOf(step.getId()));
-                if (logForNode != null) {
-                    count += logForNode.writeLogTo(0, spool);
-                }
-            }
-
-            if (count > 0) {
-                rsp.setContentType("text/plain;charset=UTF-8");
-                try (Writer writer = rsp.getWriter()) {
-                    spool.flush();
-                    spool.writeTo(new LineEndNormalizingWriter(writer));
-                }
+        for (PipelineStep step : steps.getSteps()) {
+            logText = getLogForNode(step.getId());
+            if (logText != null) {
+                foundLogs = true;
+                logText.writeLogTo(0L, rsp.getOutputStream());
             }
         }
-
-        if (logText != null) {
-            return HttpResponses.text(PipelineNodeUtil.convertLogToString(logText));
+        if (!foundLogs) {
+            rsp.getWriter().write("No logs found\n");
         }
-        return HttpResponses.text("No logs found");
     }
 
     /*
      * The default behavior of this functions differs from 'getConsoleOutput' in that it will use LOG_THRESHOLD from the end of the string.
      * Note: if 'startByte' is negative and falls outside of the console text then we will start from byte 0.
+     *
+     * FIXME: This is not performant and needs to be re-written to not buffer in memory. Avoiding JSON for log text.
+     *
      * Example:
      * {
      *   "startByte": 0,
@@ -215,7 +209,7 @@ protected JSONObject getConsoleOutputJson(String nodeId, Long requestStartByte)
                 startByte = requestStartByte;
             }
             logger.debug("Returning '{}' bytes from 'getConsoleOutput'.", textLength - startByte);
-            text = PipelineNodeUtil.convertLogToString(logText, startByte, true);
+            text = PipelineNodeUtil.convertLogToString(logText, startByte);
             endByte = textLength;
         }
         // If has an exception, return the exception text (inc. stacktrace).

src/main/java/io/jenkins/plugins/pipelinegraphview/utils/PipelineNodeUtil.java

@@ -4,7 +4,7 @@
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import edu.umd.cs.findbugs.annotations.Nullable;
-import edu.umd.cs.findbugs.annotations.SuppressWarnings;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.AbortException;
 import hudson.console.AnnotatedLargeText;
 import hudson.model.Action;
@@ -292,18 +292,7 @@ public static AnnotatedLargeText<? extends FlowNode> getLogText(@Nullable FlowNo
 
     /*
      * Get the generated log text for a given node.
-     *
-     * @param log The AnnotatedLargeText object for a given node.
-     *
-     * @return The AnnotatedLargeText object representing the log text for this
-     * node, or null.
-     */
-    public static String convertLogToString(AnnotatedLargeText<? extends FlowNode> log) throws IOException {
-        return convertLogToString(log, 0L, false);
-    }
-
-    /*
-     * Get the generated log text for a given node.
+     * FIXME: This is not performant and needs to be re-written to not buffer in memory.
      *
      * @param log The AnnotatedLargeText object for a given node.
      *
@@ -312,17 +301,14 @@ public static String convertLogToString(AnnotatedLargeText<? extends FlowNode> l
      * @return The AnnotatedLargeText object representing the log text for this
      * node, or null.
      */
-    @SuppressWarnings("RV_RETURN_VALUE_IGNORED")
-    public static String convertLogToString(AnnotatedLargeText<? extends FlowNode> log, Long startByte, boolean html)
+    @SuppressWarnings("ResultOfMethodCallIgnored")
+    @SuppressFBWarnings("RV_RETURN_VALUE_IGNORED")
+    public static String convertLogToString(AnnotatedLargeText<? extends FlowNode> log, Long startByte)
             throws IOException {
         Writer stringWriter = new StringBuilderWriter();
         // NOTE: This returns the total length of the console log, not the received
         // bytes.
-        if (html) {
-            log.writeHtmlTo(startByte, stringWriter);
-        } else {
-            log.writeLogTo(startByte, stringWriter);
-        }
+        log.writeHtmlTo(startByte, stringWriter);
         return stringWriter.toString();
     }
 

src/test/java/io/jenkins/plugins/pipelinegraphview/utils/PipelineNodeUtilTest.java

@@ -5,7 +5,6 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
-import hudson.console.AnnotatedLargeText;
 import hudson.model.Result;
 import io.jenkins.plugins.pipelinegraphview.treescanner.PipelineNodeGraphAdapter;
 import java.util.List;
@@ -19,22 +18,6 @@
 @WithJenkins
 class PipelineNodeUtilTest {
 
-    @Issue("GH#224")
-    @Test
-    void canGetLogTextFromStep(JenkinsRule j) throws Exception {
-        WorkflowRun run = TestUtils.createAndRunJob(
-                j, "hello_world_scripted", "helloWorldScriptedPipeline.jenkinsfile", Result.SUCCESS);
-
-        PipelineNodeGraphAdapter builder = new PipelineNodeGraphAdapter(run);
-        String stageId =
-                TestUtils.getNodesByDisplayName(run, "Say Hello").get(0).getId();
-        List<FlowNodeWrapper> stepNodes = builder.getStageSteps(stageId);
-        FlowNodeWrapper echoStep = stepNodes.get(0);
-        AnnotatedLargeText<? extends FlowNode> logText = PipelineNodeUtil.getLogText(echoStep.getNode());
-        String logString = PipelineNodeUtil.convertLogToString(logText);
-        assertThat(logString, equalTo("Hello, World!" + System.lineSeparator()));
-    }
-
     @Issue("GH#224")
     @Test
     void canGetErrorTextFromStep(JenkinsRule j) throws Exception {