CAMEL-24045: camel-sql - Fix connection leak with outputType=StreamList when the statement is not a query#24656
Conversation
…st when the statement is not a query
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean, minimal fix for a genuine connection leak in SqlProducer.processStreamList() when the executed statement is not a query (INSERT/UPDATE/DELETE/DDL). The fix correctly closes the statement and connection in the previously-missing else branch.
Scanner coverage: No static analysis tools (PMD, Checkstyle, semgrep, SpotBugs) available. Rely on CI.
Notes:
- The fix correctly relies on Spring's
JdbcUtils.closeStatement()andJdbcUtils.closeConnection(), both of which handle nulls and swallow exceptions internally, so sequential close calls are safe. - The test is well-crafted: the
ConnectionCountingDataSourceusing JDK Proxy tracks connection lifecycle and verifies both the functional outcome and the resource invariant (zero open connections).
One minor observation: the new else branch does not set the SQL_UPDATE_COUNT header, unlike the equivalent non-query path in processInternal(). For completeness, consider adding exchange.getIn().setHeader(SqlConstants.SQL_UPDATE_COUNT, ps.getUpdateCount()) before closing the statement. This is low severity since StreamList mode with non-queries is an edge case.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code review on behalf of @gnodet
… StreamList statements
|
Thanks @gnodet I've added the header |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Reviewed with focus on resource lifecycle across all execution paths — the fix is correct and I have no change requests. Summary of the verification:
The bug is real and the fix closes exactly the leaked path. On current main, processStreamList (SqlProducer.java:260-269) only handles ps.execute() == true: the ResultSet path hands con/ps/rs ownership to ResultSetIterator and defers cleanup to ResultSetIteratorCompletion at exchange completion. When the statement is not a query (execute() returns false), the method returns a null iterator and con/ps are never closed — the catch block (SqlProducer.java:272-274) only covers the exception path. The new else { closeStatement(ps); closeConnection(con); } branch closes them exactly once, cannot interfere with the streaming path (it only runs when there is no ResultSet to stream), and cannot double-close on exception (Spring's JdbcUtils.close* never throw, so the branch can't re-enter the catch).
No double-close on the streaming side either: ResultSetIterator.close() is idempotent via AtomicBoolean.compareAndSet, which covers the self-close-on-exhaustion and empty-result-set cases.
Test quality is a highlight: the regression test asserts the actual invariant (openConnections() == 0 via a counting DelegatingDataSource proxy) rather than merely exercising the path, is fully synchronous (no Thread.sleep, native assertIsSatisfied), and fails on unfixed main by inspection.
History check: the missing else traces back to the original CAMEL-9849 StreamList implementation — there is no prior intentional decision to keep connections open on non-query statements, so this does not revert deliberate behavior. Behavior parity is preserved (body/headers untouched on the non-query path, as before; the absent CamelSqlUpdateCount on this path is pre-existing and would be a separate enhancement).
Two merge-time notes:
- CI: the two build jobs (JDK 17/25) were still pending at review time — worth confirming green before merge.
- The PR body declares "Generated with Claude Code" but the commit has no AI co-authorship trailer — per the repo's AI guidelines the squash-merge commit message should include the
Co-authored-bytrailer for the AI assistant.
Reviewed with Claude Code (Fable 5) on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Re-review focused on the new commit (2864ca45): CAMEL-24045: camel-sql - Set CamelSqlUpdateCount header for non-query StreamList statements
This commit adds exactly two lines and directly implements the suggestion from the prior review:
-
SqlProducer.java: SetsCamelSqlUpdateCountviaps.getUpdateCount()in the non-queryelsebranch ofprocessStreamList(), beforecloseStatement(ps)— the ordering is correct since the update count is not accessible after the statement is closed. -
Test: Adds
mock.expectedHeaderReceived(SqlConstants.SQL_UPDATE_COUNT, 1)asserting the header value matches the expected single-row update.
The pattern is consistent with the equivalent non-query path in processInternal() (line 218). No issues found.
Scanner coverage: No standalone static analysis tools (PMD, Checkstyle, semgrep, SpotBugs) available. Relying on CI for validation.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Re-review focused on the new commit (2864ca45): CAMEL-24045: camel-sql - Set CamelSqlUpdateCount header for non-query StreamList statements
This commit adds exactly two lines and directly implements the suggestion from the prior review:
-
SqlProducer.java: SetsCamelSqlUpdateCountviaps.getUpdateCount()in the non-queryelsebranch ofprocessStreamList(), beforecloseStatement(ps)— the ordering is correct since the update count is not accessible after the statement is closed. -
Test: Adds
mock.expectedHeaderReceived(SqlConstants.SQL_UPDATE_COUNT, 1)asserting the header value matches the expected single-row update.
The pattern is consistent with the equivalent non-query path in processInternal() (line 218). No issues found.
Scanner coverage: No standalone static analysis tools (PMD, Checkstyle, semgrep, SpotBugs) available. Relying on CI for validation.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 tested, 29 compile-only — current: 11 all testedMaveniverse Scalpel detected 40 affected modules (current approach: 11).
|
4 participants
JIRA: https://issues.apache.org/jira/browse/CAMEL-24045
SqlProducer.processStreamList()takes a raw connection from the DataSource so the result set can stay open while the exchange is routed. WhenPreparedStatement.execute()returnsfalse(INSERT/UPDATE/DELETE/DDL), the result-set branch was skipped: noResultSetIteratorCompletionwas registered and neither the connection nor the statement was ever closed — the catch block only cleaned up on exception. Since the executed SQL can vary per exchange via theCamelSqlQueryheader oruseMessageBodyForSql=true, each such exchange leaked one pooled connection, eventually exhausting the pool.Changes:
CamelSqlUpdateCountheader fromgetUpdateCount()for the non-query case, consistent with the non-query path inprocessInternal()(review follow-up)main, passes with the fix); existing StreamList producer/consumer tests unaffectedClaude Code on behalf of Croway
🤖 Generated with Claude Code