ClosingStream should close streams on thrown exception - #52582
Conversation
Closes keycloak#52580 Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR ensures ClosingStream and its primitive variants reliably close their underlying streams even when terminal operations throw, preventing resource leaks (e.g., JDBC ResultSet cursors).
Changes:
- Wrapped terminal operations in
try/finallyto guaranteeclose()executes on success or failure. - Updated iterator/spliterator traversal (
hasNext,tryAdvance,forEachRemaining) to close on exhaustion and on thrown exceptions. - Added JUnit tests validating closure behavior when terminal operations fail.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server-spi-private/src/main/java/org/keycloak/utils/ClosingStream.java | Ensures close-on-exception for terminal ops; closes on iterator/spliterator errors and exhaustion. |
| server-spi-private/src/main/java/org/keycloak/utils/ClosingIntStream.java | Applies the same close-on-exception behavior to IntStream terminals and traversal helpers. |
| server-spi-private/src/main/java/org/keycloak/utils/ClosingLongStream.java | Applies the same close-on-exception behavior to LongStream terminals and traversal helpers. |
| server-spi-private/src/main/java/org/keycloak/utils/ClosingDoubleStream.java | Applies the same close-on-exception behavior to DoubleStream terminals and traversal helpers. |
| server-spi-private/src/test/java/org/keycloak/utils/StreamsUtilTest.java | Adds regression tests for closing behavior when terminal operations throw. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
| DoubleStream result = delegate.asDoubleStream(); | ||
| close(); | ||
| return result; | ||
| return new ClosingDoubleStream(delegate.asDoubleStream()); |
| try { | ||
| delegate.forEach(action); | ||
| } finally { | ||
| close(); | ||
| } |
| boolean res; | ||
| try { | ||
| res = iterator.hasNext(); | ||
| } catch (RuntimeException | Error e) { | ||
| close(); | ||
| throw e; | ||
| } |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.actions.TermsAndConditionsTest#termsAcceptedKeycloak CI - Forms IT (firefox) |
Closes #52580
Summary
Ensure
ClosingStream,ClosingIntStream,ClosingLongStream, andClosingDoubleStreamclose the underlying stream when a terminal operation throws an exception. Previously, exceptions during terminal operations would bypassclose(), leaking JDBCResultSetcursors.Also fixes
asLongStream(),asDoubleStream()inClosingIntStreamandasDoubleStream()inClosingLongStreamwhich were incorrectly treated as terminal operations instead of intermediate operations.Decisions and assumptions
try-finallyfor all terminal operations: Every terminal method (forEach,toArray,reduce,collect,count,min,max,anyMatch,allMatch,noneMatch,findFirst,findAny,toList,sum,average) wraps the delegate call intry-finallyto guaranteeclose()runs regardless of success or failure.try-catchforhasNext()andtryAdvance(): These methods are called repeatedly — closing on every call would break iteration. Instead, they close on exception (viatry-catch) and on exhaustion (when returningfalse). This preserves the existing behavior of closing when the stream is fully consumed.try-finallyforforEachRemaining(): BothClosingIterator.forEachRemaining()andClosingSpliterator.forEachRemaining()consume all remaining elements in a single call, sotry-finallyis appropriate here.No change to
next():Iterator.next()is always preceded byhasNext(), which already handles close-on-error and close-on-exhaustion.asLongStream()/asDoubleStream()are intermediate, not terminal: These were incorrectly closing the underlying stream and returning an unwrapped stream. Fixed to returnClosingLongStream/ClosingDoubleStreamwrappers, consistent withboxed().No suppressed exception handling in
try-finally: If both the terminal operation andclose()throw, the close exception masks the original. This matches standardtry-finallysemantics and is acceptable becauseStream.close()internally catches and aggregates exceptions fromonClose()handlers, making a throwingclose()extremely unlikely.All four stream classes updated consistently: The same pattern is applied to
ClosingStream<R>,ClosingIntStream,ClosingLongStream, andClosingDoubleStream.Tests cover all stream types: Exception-on-terminal tests added for
ClosingStream,ClosingIntStream,ClosingLongStream, andClosingDoubleStream. Additional tests verify thatasLongStream()andasDoubleStream()preserve close propagation.