fix(#4852): resolve OutOfMemoryError for large files in Cache.java#4869
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
There was a problem hiding this comment.
Pull request overview
This PR addresses OutOfMemoryError risk in Cache hashing by switching SHA-256 computation from loading full file bytes into memory to a streaming approach, and updates tests around SHA generation.
Changes:
- Replace
Files.readAllBytes(...)hashing with buffered streaming hashing inCache.sha(...). - Remove the related puzzle/todo from
Cache.java. - Add new tests that assert SHA-256 hash file contents for “large” and tiny inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java | Streams file content into MessageDigest to avoid hashing large files via readAllBytes. |
| eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java | Adds additional SHA-256 verification tests for larger and tiny content sizes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final int lines = 100_000; | ||
| final StringBuilder builder = new StringBuilder(lines * 10); | ||
| IntStream.range(0, lines).forEach( | ||
| i -> builder.append("Line ").append(i).append('\n') | ||
| ); | ||
| final String content = builder.toString(); | ||
| Files.writeString(source, content, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
The "large file" test still builds the entire file content in memory (StringBuilder + content String + content.getBytes for expected digest). That makes the test itself memory-heavy and it won’t catch a regression back to Files.readAllBytes (the old implementation would still pass at this size). Consider generating/writing the file incrementally and computing the expected SHA-256 via a streaming digest as well, so the test stays low-memory and can be scaled without holding the full content in RAM.
| @Test | ||
| void generatesCorrectHashForTinyFile( | ||
| @Mktmp final Path temp | ||
| ) throws IOException, NoSuchAlgorithmException { | ||
| final var cache = temp.resolve("cache"); | ||
| Files.createDirectories(cache); | ||
| final var source = temp.resolve("tiny.txt"); | ||
| final String content = "x"; | ||
| Files.writeString(source, content, StandardCharsets.UTF_8); | ||
| final var target = temp.resolve("out.txt"); | ||
| final var tail = source.getFileName(); | ||
| new Cache(cache, p -> content).apply(source, target, tail); | ||
| MatcherAssert.assertThat( | ||
| "SHA-256 hash file has incorrect content for tiny file", | ||
| Files.readString( | ||
| cache.resolve(String.format("%s.sha256", tail)), | ||
| StandardCharsets.UTF_8 | ||
| ), | ||
| Matchers.equalTo( | ||
| Base64.getEncoder().encodeToString( | ||
| MessageDigest.getInstance("SHA-256") | ||
| .digest(content.getBytes(StandardCharsets.UTF_8)) | ||
| ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
The new tiny-file hash test largely overlaps with the existing writesCorrectShaHash() coverage (both validate SHA-256 file contents for a small input). Unless it’s guarding a specific edge case, consider removing it or turning these into a single parameterized test to avoid duplicated setup/logic.
|
@yegor256 could you have a look, please? |
|
@yegor256 friendly reminder |
|
@volodya-lombrozo Thanks for the contribution! You've earned +16 points for your code submission. This reward follows our team policy which awards 16 points for reviews provided, plus additional points based on your hits-of-code contribution. Please keep them coming! Your running score is +446; don't forget to check your Zerocracy account too. |
This PR resolves the
OutOfMemoryErrorissue inCache.javawhen handling large files.Fixes #4852
Summary by CodeRabbit