Stream HTML console log

[das7pad]

• Part of Viewing large logs runs out of memory pipeline-graph-view-plugin#704
• Based on Add new LargeText interface based constructor for AnnotatedLargeText #11071
• Adopts Add new streaming mode for LargeText stapler#722

This PR is adopting the streaming/unbuffered reads from staplers LargeText in AnnotatedLargeText and integrates it with the progressive-text in the frontend.

Before: start of request; read all the requested log bytes into memory; emit response with start/end/annotation state in headers
After: start of request; stream the requested log bytes into multi-part response; emit another multi-part part with start/end/annotation state

Testing done

See updated tests for the log/progressiveHtml endpoint. Open console while build is in progress.

Proposed changelog entries

• Stream HTML console log

Proposed changelog category

/label rfe

Proposed upgrade guidelines

N/A

Submitter checklist

• The Jira issue, if it exists, is well-described.
• The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
• There is automated testing or an explanation as to why this change has no tests.
• New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
• New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
• New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
• For dependency updates, there are links to external changelogs and, if possible, full differentials.
• For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

cc @timja @jglick

Before the changes are marked as ready-for-merge:

Maintainer checklist

• There are at least two (2) approvals for the pull request and no outstanding requests for change.
• Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
• Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
• Proper changelog labels are set so that the changelog can be generated automatically.
• If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!

[das7pad]

Thanks for the feedback, Tim. I've addressed it in 831b743.

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!

[das7pad]

The stapler upgrade with jenkinsci/stapler#722 is in #11221. Once that's merged, I can then remove the incremenal for stapler and mark this PR as ready for review.

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!

[jglick]

Looks OK though I did not test it.

[timja]

Looks reasonable, untested.

---

/label ready-for-merge

---

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

[jglick]

Checked with a simple Pipeline build that printed output at intervals, using built-in console viewer, on Firefox on Chrome. Works, though I am a bit confused since it is still opening a fresh HTTP request every second or so to …/logText/progressiveHtml even though it is using chunked output. I thought the point of the new transport was to reuse one HTTP connection for many chunks? Or does that only apply to viewing a completed build (or a sudden very large blob of text)? When I view a completed build, the full text is rendered in the main HTTP response …/console. Perhaps I misunderstanding what this PR is supposed to do?

[das7pad]

The "streaming" applies to the server side request/response cycle. Previously, logText/progressiveHtml would buffer the entire response body into memory before emitting it. Now, it starts writing bytes to the wire as soon as they are read and processed for annotations.

Before: start of request; read all the requested log bytes into memory; emit response with start/end/annotation state in headers
After: start of request; stream the requested log bytes into multi-part response; emit another multi-part part with start/end/annotation state

[jglick]

Ah OK, so the multipart content type here is not used to send multiple chunks of text, merely to work around the fact that HTTP does not permit response headers to be written after content.

[das7pad]

Ah OK, so the multipart content type here is not used to send multiple chunks of text, merely to work around the fact that HTTP does not permit response headers to be written after content.

Yes.

Side note: There are Trailers in HTTP, but support for them remains very limited -- notably browsers do not expose them.

[A1exKH]

LGTM.

[timja]

Plugin compat tests failing, could you take a look please?

https://ci.jenkins.io/job/Tools/job/bom/job/PR-5906/1/testReport/org.jenkinsci.plugins.workflow.support.steps.input/InputStepTest/pct_pipeline_input_step_plugin_weekly___test_submitter_parameter/

https://ci.jenkins.io/job/Tools/job/bom/job/PR-5906/1/testReport/org.jenkinsci.plugins.workflow.support.steps.input/InputStepTest/pct_pipeline_input_step_plugin_weekly___test_submitter_parameter_no_submitter/

[jglick]

java.lang.NumberFormatException: For input string: "null"
	at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:67)
	at java.base/java.lang.Long.parseLong(Long.java:709)
	at java.base/java.lang.Long.parseLong(Long.java:832)
	at org.kohsuke.stapler.framework.io.LargeText.doProgressTextStreaming(LargeText.java:402)
	at org.kohsuke.stapler.framework.io.LargeText.doProgressTextImpl(LargeText.java:435)
	at org.kohsuke.stapler.framework.io.LargeText.doProgressText(LargeText.java:319)
	at hudson.console.AnnotatedLargeText.doProgressiveHtmlImpl(AnnotatedLargeText.java:124)
	at hudson.console.AnnotatedLargeText.doProgressiveHtml(AnnotatedLargeText.java:109)

@das7pad PTAL

[jglick]

(We could really use a PR label on core & plugin PRs forcing a bom run. You can do it manually by waiting for Incremental deployment and then filing a draft weekly-test PR but it is cumbersome.)

[das7pad]

https://ci.jenkins.io/job/Tools/job/bom/job/PR-5906/1/testReport/org.jenkinsci.plugins.workflow.support.steps.input/InputStepTest/pct_pipeline_input_step_plugin_weekly___test_submitter_parameter/

Looks good to me in the browser (after putting a sleep in the broken test).

   7.797 [id=892]	SEVERE	o.h.j.DefaultJavaScriptErrorListener#scriptException: Error during JavaScript execution
======= EXCEPTION START ========
Exception class=[org.htmlunit.corejs.javascript.EvaluatorException]
org.htmlunit.ScriptException: invalid property id (http://localhost:41875/jenkins/adjuncts/2a56c92f/lib/hudson/progressive-text.js#70)
	at org.htmlunit.javascript.JavaScriptEngine$HtmlUnitCompileContextAction.run(JavaScriptEngine.java:900)

Looking at the full trace and seeing loads of org.htmlunit.corejs.javascript.Parser.expExpr makes me wonder if the installed version of org.htmlunit.corejs.javascript is lacking support for Response.formData(), which progressive-text is now using. progressive-text.js#70 is very close to line 67 where we call response.formData().

[das7pad]

java.lang.NumberFormatException: For input string: "null"
	at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:67)
	at java.base/java.lang.Long.parseLong(Long.java:709)
	at java.base/java.lang.Long.parseLong(Long.java:832)
	at org.kohsuke.stapler.framework.io.LargeText.doProgressTextStreaming(LargeText.java:402)
	at org.kohsuke.stapler.framework.io.LargeText.doProgressTextImpl(LargeText.java:435)
	at org.kohsuke.stapler.framework.io.LargeText.doProgressText(LargeText.java:319)
	at hudson.console.AnnotatedLargeText.doProgressiveHtmlImpl(AnnotatedLargeText.java:124)
	at hudson.console.AnnotatedLargeText.doProgressiveHtml(AnnotatedLargeText.java:109)

@jglick From which test is this?

From the trace, it would be parsing the ?start parameter. Here is the code that sets the initial start for progressive-text in Jenkins:

jenkins/core/src/main/resources/lib/hudson/progressive-text.js

Lines 110 to 112 in 4702ffe

	document.getElementById(idref).fetchedBytes =
	startOffset !== "" ? Number(startOffset) : 0;
	fetchNext(document.getElementById(idref), href, onFinishEvent);

Under which condition can Number(...) yield "null" 🤔

> new URLSearchParams({ start: Number(undefined) }).toString()
'start=NaN'
> new URLSearchParams({ start: Number(null) }).toString()
'start=0'
> new URLSearchParams({ start: Number('null') }).toString()
'start=NaN'
> new URLSearchParams({ start: Number('undefined') }).toString()
'start=NaN'

[jglick]

From which test is this?

Both of the ones observed to fail, links in #11177 (comment)

You can reproduce failures yourself locally:

cd pipeline-input-step-plugin
mvnd test -Dtest=InputStepTest -Djenkins.version=2.534

I see

invalid property id (http://localhost:44373/jenkins/adjuncts/da1a6239/lib/hudson/progressive-text.js#70)

With 2.533 it passes.

A limitation in the JavaScript engine in HtmlUnit would not be all that surprising (jenkinsci/jenkins-test-harness#569), but then in the short term we need a way for the JS to detect the impoverished test environment and automatically fall back to the older implementation.

[jglick]

Under which condition can Number(...) yield "null"

I am no JS expert but it strikes me as more likely that when

jenkins/core/src/main/resources/lib/hudson/progressive-text.js

Line 36 in f9a3156

start: e.fetchedBytes,

is run, fetchedBytes has not been defined at all, and null is being coerced to "null".

[das7pad]

Under which condition can Number(...) yield "null"

I am no JS expert but it strikes me as more likely that when

jenkins/core/src/main/resources/lib/hudson/progressive-text.js

Line 36 in f9a3156

start: e.fetchedBytes,

is run, fetchedBytes has not been defined at all, and null is being coerced to "null".

e is document.getElementById(idref), on which we set fetchedBytes before calling fetchNext the first time.

jenkins/core/src/main/resources/lib/hudson/progressive-text.js

Lines 110 to 112 in 4702ffe

	document.getElementById(idref).fetchedBytes =
	startOffset !== "" ? Number(startOffset) : 0;
	fetchNext(document.getElementById(idref), href, onFinishEvent);

[das7pad]

I've added basic support for multipart/form-data responses in htmlunit (HtmlUnit/htmlunit#1054).

After updating Jenkins, bom and jenkins-test-harness, and pulling down the updated fetch polyfill, the tests are passing in pipeline-input-step-plugin 🎉

I've opened jenkinsci/jenkins-test-harness-htmlunit#198 for pulling in the htmlunit patch right away, but I cannot get the build green. Any help will be much appreciated!

---

For local testing:

cd pipeline-input-step-plugin
wget https://raw.githubusercontent.com/das7pad/htmlunit/db04ce08d77806092607b7751e9be13660f5d7c9/src/main/resources/org/htmlunit/javascript/polyfill/fetch/fetch.umd.js
mkdir -p src/test/resources/org/htmlunit/javascript/polyfill/fetch/
mv fetch.umd.js src/test/resources/org/htmlunit/javascript/polyfill/fetch/

patch for upgrades

diff --git a/pom.xml b/pom.xml
index 42771ff..379c2ad 100644
--- a/pom.xml
+++ b/pom.xml
@@ -39,8 +39,8 @@
     <properties>
         <changelist>999999-SNAPSHOT</changelist>
         <!-- https://www.jenkins.io/doc/developer/plugin-development/choosing-jenkins-baseline/ -->
-        <jenkins.baseline>2.479</jenkins.baseline>
-        <jenkins.version>${jenkins.baseline}.1</jenkins.version>
+        <jenkins.baseline>2.528</jenkins.baseline>
+        <jenkins.version>2.534</jenkins.version>
         <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
         <hpi.strictBundledArtifacts>true</hpi.strictBundledArtifacts>
     </properties>
@@ -49,7 +49,7 @@
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
                 <artifactId>bom-${jenkins.baseline}.x</artifactId>
-                <version>3893.v213a_42768d35</version>
+                <version>5601.v59f37270a_349</version>
                 <type>pom</type>
                 <scope>import</scope>
             </dependency>
@@ -122,6 +122,12 @@
             <artifactId>credentials-binding</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.main</groupId>
+            <artifactId>jenkins-test-harness</artifactId>
+            <version>2516.vc8fe6b_362b_19</version>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.awaitility</groupId>
             <artifactId>awaitility</artifactId>