Fix BufferedReader resource leak in InputStreamConsumer#18469
Conversation
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for contributing! The intent of the fix is correct — converting to try-with-resources is the right approach for this resource leak. However, there's a stray ) on line 43 that introduces a syntax error and is blocking CI (the Azure build is already showing FAILURE). Just a one-character fix needed before this can land.
| BufferedReader br = new BufferedReader(isr); | ||
| try (InputStreamReader isr = new InputStreamReader(is, StandardCharsets.UTF_8); | ||
| BufferedReader br = new BufferedReader(isr)) {) | ||
| br.lines().forEach(log::info); |
There was a problem hiding this comment.
🤖 There's a stray ) at the end of this line — )) {) — which is a syntax error and will prevent compilation. It should be )) { (no trailing paren). This is confirmed by the CI failure on this PR. Could you drop that extra ) and push a fix?
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
LGTM — clean fix that correctly converts the plain try block to try-with-resources, ensuring both the InputStreamReader and BufferedReader are closed on both normal and exceptional exit.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18469 +/- ##
============================================
+ Coverage 68.53% 68.77% +0.23%
- Complexity 28037 28131 +94
============================================
Files 2444 2451 +7
Lines 134752 134986 +234
Branches 16292 16361 +69
============================================
+ Hits 92356 92836 +480
+ Misses 35112 34799 -313
- Partials 7284 7351 +67
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Describe the issue this Pull Request addresses
Problem
InputStreamConsumer.run()creates anInputStreamReaderandBufferedReaderinside a plaintryblock. If an exception occurs duringbr.lines().forEach(...), neither reader is closed, causing a resource leak.Solution
Convert the plain
tryblock to a try-with-resources statement that declares bothInputStreamReaderandBufferedReaderas managed resources, ensuring they are automatically closed on both normal and exceptional exit.Changes
hudi-cli/src/main/java/org/apache/hudi/cli/utils/InputStreamConsumer.java: WrapInputStreamReaderandBufferedReaderconstruction in a try-with-resources statement in therun()method.Summary and Changelog
Impact
Risk Level
Documentation Update
Contributor's checklist