Skip to content

Issue #1039: Fix OutOfMemoryError in postProcessCheckstyleReport by r…#1044

Open
vivek-0509 wants to merge 1 commit intocheckstyle:masterfrom
vivek-0509:issue-1039-fix-oom-in-post-process-checkstyle-report
Open

Issue #1039: Fix OutOfMemoryError in postProcessCheckstyleReport by r…#1044
vivek-0509 wants to merge 1 commit intocheckstyle:masterfrom
vivek-0509:issue-1039-fix-oom-in-post-process-checkstyle-report

Conversation

@vivek-0509
Copy link
Contributor

Issue #1039:

Fix OutOfMemoryError for large checkstyle-result.xml files by streaming line-by-line instead of loading entire file into memory

@vivek-0509 vivek-0509 force-pushed the issue-1039-fix-oom-in-post-process-checkstyle-report branch from bae8f59 to 01e42b9 Compare March 12, 2026 10:10
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

item:

writer.writeLine(line.replace(oldPath, newPath))
}
}
Files.move(tempFile.toPath(), checkstyleResultFile.toPath(), REPLACE_EXISTING)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can it be?

import java.nio.file.Files
Files.copy(sourcePath, destPath, StandardCopyOption.REPLACE_EXISTING)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romani
Yes, we can but i thought of using Files.move over Files.copy because it automatically removes the temp file after the operation, no manual tempFile.delete() needed. With Files.copy, the temp file remains on disk, if we do not clean. It would be helpful if I could get some clarity on why Files.copy is preferred here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No preference. Just tried to use already written standard method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure will use Files.copy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romani done

…eport by replacing whole-file read with line-by-line streaming
@vivek-0509 vivek-0509 force-pushed the issue-1039-fix-oom-in-post-process-checkstyle-report branch from 01e42b9 to aac1e8d Compare March 12, 2026 13:45
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Items

checkstyleResultFile.text = content
tempFile.withWriter { writer ->
checkstyleResultFile.eachLine { line ->
writer.writeLine(line.replace(oldPath, newPath))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, ....

I started above discussion on this code.
Why we need such custom each line read write?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is needed because the original file.text loads the entire file into a single String, which causes OutOfMemoryError for large files, Error we saw earlier, i find eachLine/withWriter as the simplest streaming solution in Groovy that avoids loading the whole file into memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants