-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
Attempt to fix logText progressive memory issue in LargeText.java #10236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempt to fix logText progressive memory issue in LargeText.java #10236
Conversation
|
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
A1exKH
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kalindafab LGTM.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-server</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
| ObjectOutputStream oos = AnonymousClassWarnings.checkingObjectOutputStream(new GZIPOutputStream(new CipherOutputStream(baos, sym))); | ||
| oos.writeLong(System.currentTimeMillis()); // send timestamp to prevent a replay attack | ||
| ObjectOutputStream oos = AnonymousClassWarnings.checkingObjectOutputStream( | ||
| new GZIPOutputStream(new CipherOutputStream(baos, sym)) | ||
| ); | ||
|
|
||
| oos.writeLong(System.currentTimeMillis()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to just be formatting changes (and deleting a comment)? Please try to keep diffs minimal.
| if (pos >= chunkSize) { | ||
| in.skip(2); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted to chunk the logs while reading them by checking how much data has been read using a pos >= chunkSize check. When that threshold is reached, I used in.skip(2) to move past the delimiter or line break ( \r\n) between log entries.
The intention here was to read the logs in manageable chunks without holding the entire log in memory, and to prepare for streaming output more efficiently.
|
I am surprised to see a proposed fix in this repo (other than a dependency bump), when the problem appears to be in https://github.com/jenkinsci/stapler/blob/fcb700b75e9ed613d7d58e47aa117f504d2affe6/core/src/main/java/org/kohsuke/stapler/framework/io/LargeText.java#L310-L319 which is attempting to first write the entire log text to a memory buffer and then stream it, which is obviously not what we want. If we did not bother sending
writeLogTo a special counting Writer that discards content, then set the header, then write again, but this seems inefficient. Depending on the Source we could do better., I think.
Note that https://github.com/jenkinsci/blueocean-plugin/blob/ab3a0465c4e68be8247158ae3c7733e88094d5a3/blueocean-rest-impl/src/main/java/io/jenkins/blueocean/service/embedded/rest/LogResource.java#L93 duplicates code but this is used only in Blue Ocean, which is not maintained anyway. https://github.com/jenkinsci/pipeline-graph-view-plugin/blob/01dba0944a687b2ad5487ba8d0db49c1146458ff/src/main/java/io/jenkins/plugins/pipelinegraphview/consoleview/PipelineConsoleViewAction.java#L122-L138 also looks suspicious. The whole design of |
|
jenkinsci/stapler#657 might work but I did not try to test it yet. |
|
@jglick |
|
maybe it would significantly reduce memory usage for large logs while keeping backward compatibility where needed. Also, making createWriter() public or allowing configurable streaming behavior would help plugin developers avoid duplicating flawed logic. |
That is what my PR attempts to do.
I took a different approach, since the current design only supports two content providers (file and byte buffer) both of which have a known length which can be looked up in advance. |
jglick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe #10515 solves the problem more directly; please help test if you can.
right |
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
sorry! i might delay because i don't have enough time to work on this |
See (https://issues.jenkins.io/browse/JENKINS-75081)
Testing done
I tested this change by running a Jenkins pipeline that generates a large amount of log output.
Tested with 7GB logs: The logs streamed correctly without an OutOfMemoryError.
Tested with 10GB logs: The issue still occurs, showing the same large log behavior.
Heap memory usage monitored: Observed heap dump and confirmed the Jetty thread still holds logs in memory.
Manual verification: Accessed /logText/progressiveText API after each build and checked the response.
No automated tests added because this issue requires large-scale log generation that is difficult to replicate in unit tests.
Proposed changelog entries
Improve /logText/progressiveText API to handle large logs more efficiently and reduce heap memory usage.
Stream log data in smaller chunks instead of loading large portions into memory at once.
Modify LargeText and AnnotatedLargeText to avoid excessive memory consumption for logs above 7GB.
Partial fix for heap exhaustion in large build logs; further improvements needed for logs exceeding 10GB.
The fix works for logs up to 7GB, but the issue still persists for logs over 10GB. I’d appreciate guidance on further improving memory handling in Jetty and any best practices for handling extremely large logs efficiently.
Looking forward to your feedback—thanks in advance! 🙌
Proposed upgrade guidelines
N/A
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge: