You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "AryamannSingh7 (via GitHub)" <gi...@apache.org> on 2026/08/18 09:48:57 UTC
[PR] Fix statement close invalidating live result sets of other statements [shardingsphere]
AryamannSingh7 opened a new pull request, #39503:
URL: https://github.com/apache/shardingsphere/pull/39503
Fixes #39189.
Changes proposed in this pull request:
- Gate the auto-commit connection cleanup in `AbstractStatementAdapter#handleAutoCommitAfterExecution` on there being no other statement still registered on the logical connection, so closing one statement no longer closes physical connections that still back another statement's live `ResultSet`.
- Add `ShardingSphereConnection#hasRegisteredStatementManagers()` to expose that state.
- Add two regression tests: one reproducing the failure, and one asserting the connections *are* still released once the last statement closes.
### Root cause
This is a 5.5.2 -> 5.5.3 regression introduced by #35834.
`AbstractStatementAdapter#close` calls `safeCloseAndUnregisterStatementManager`, which ends in `handleAutoCommitAfterExecution` -> `DriverDatabaseConnectionManager#clearCachedConnections()`. That closes **every** connection in `cachedConnections`, and that Multimap is owned by the logical connection and shared by all of its statements. Since a `StatementManager` is created per `Statement`, closing a secondary statement closes the physical connections still backing an outer statement's open `ResultSet`.
It is not a Hibernate problem — Hibernate just produces the access pattern that triggers it. It reproduces with plain JDBC and two statements on one connection, which is what the added test does.
The `connection.getAutoCommit()` gate on that call also explains both of the reporter's observations: 5.5.2 works because the path did not exist before #35834, and `@Transactional(readOnly = true)` works because inside a transaction `autoCommit` is false, so the clear never runs.
### Fix
`unregisterStatementManager` already runs immediately before the auto-commit handling, so at that point a non-empty registry means another statement is still open:
```java
protected final void handleAutoCommitAfterExecution(final ShardingSphereConnection connection) throws SQLException {
if (connection.getAutoCommit() && !connection.hasRegisteredStatementManagers()) {
connection.getDatabaseConnectionManager().clearCachedConnections();
}
}
```
This preserves #35834's timely-release goal — the last statement to close still releases everything — and only skips the clear while another statement is genuinely live.
### Verification
Run on `jdbc` against current `master`:
- `mvn -pl jdbc test` — **1216 tests, 0 failures**, including the tests added by #39437 to `StatementAdapterTest` and `DriverDatabaseConnectionManagerTest`.
- `mvn -Pcheck -pl jdbc validate` — 0 Checkstyle violations; `spotless:check` clean.
- A/B checked both ways: with the guard reverted, `assertResultSetRemainsUsableAfterAnotherStatementCloses` fails while `assertCachedConnectionsClearedWhenLastStatementCloses` still passes — so the second test is genuinely guarding the release behaviour rather than being trivially satisfied.
### Two open questions
I raised these on the issue and am happy to go either way:
1. **Test placement** — these currently live in `ShardingSphereDriverTest`. Happy to move them to a dedicated multi-statement lifecycle test class instead, as suggested in the issue.
2. **Granularity** — using open statements as the proxy for "a result set may still be live" is deliberately conservative: a statement left open without a live result set will hold its connections until it closes. Tracking open result sets directly would be tighter but considerably more invasive. Glad to go that way if preferred.
Also happy to add an E2E case if this should be covered beyond the unit level.
---
Before committing this PR, I'm sure that I have checked the following options:
- [x] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [x] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [x] I have passed maven check locally (exact scope listed under Verification above).
- [ ] I have made corresponding changes to the documentation. (No documentation impact — internal resource-lifecycle fix.)
- [x] I have added corresponding unit tests for my changes.
- [ ] I have updated the Release Notes of the current development version. (Will add the entry once this PR has a number, unless you would rather it be left to a separate release-notes PR.)
- [x] I have disclosed the tool and affected files or scope for any material AI assistance, as required by the [AI-assisted contribution policy].
**AI assistance:** Claude (Claude Code) was used while investigating and implementing this change, covering `AbstractStatementAdapter`, `ShardingSphereConnection` and `ShardingSphereDriverTest`. I have reviewed and verified all of it and can explain every line.
[AI-assisted contribution policy]: https://github.com/apache/shardingsphere/blob/master/AI_POLICY.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix statement close invalidating live result sets of other statements [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu merged PR #39503:
URL: https://github.com/apache/shardingsphere/pull/39503
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix statement close invalidating live result sets of other statements [shardingsphere]
Posted by "AryamannSingh7 (via GitHub)" <gi...@apache.org>.
AryamannSingh7 commented on PR #39503:
URL: https://github.com/apache/shardingsphere/pull/39503#issuecomment-5326495976
Could a committer add the `type: bug` and `in: JDBC` labels, matching #39189?
@terrymanu — this implements the fix for the regression you triaged on that issue. The two questions from [my analysis comment](https://github.com/apache/shardingsphere/issues/39189#issuecomment-5108865819) are still open (test-class placement, and whether the conservative open-statements proxy is acceptable); I've noted both in the description and I'm happy to reshape either in review rather than hold the PR on them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org