You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "borinquenkid (via GitHub)" <gi...@apache.org> on 2026/10/26 00:48:59 UTC
[PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
borinquenkid opened a new pull request, #16763:
URL: https://github.com/apache/grails-core/pull/16763
Fixes #16571.
## Problem
When a transaction on a request-bound Hibernate session completes, Spring's `disconnectOnCompletion(session)` disconnects the session's physical JDBC connection, allowing it to be returned to the connection pool until needed.
If the application subsequently opens a nested `withNewSession` and inside it starts a transaction (`withTransaction`), Spring's `TransactionSynchronizationManager` binds the inner transaction's physical connection to the thread under the `dataSource` resource key.
If an entity loaded earlier in the outer request session is touched inside that nested scope (for example, accessing a lazy association like `book.author.name`), the outer session detects that it is physically disconnected and requests a connection from the `dataSource`. Because the `dataSource` is wrapped in Spring's `TransactionAwareDataSourceProxy`, it returns the thread-bound connection of the inner transaction.
When the inner transaction completes, it closes its connection. The outer session is left holding that closed connection, and subsequent operations fail with:
```
java.sql.SQLException: PooledConnection has already been closed.
```
Additionally, if `convertJdbcAccessException` returned `null` because the database vendor error code could not be mapped, `GrailsHibernateTemplate.doExecute` would throw `NullPointerException` when trying to throw the result of `convertHibernateAccessException`.
## Change
1. In `GrailsHibernateTemplate.executeWithNewSession`, before unbinding the existing session holder and entering the new session scope, check whether the outer session is open but physically disconnected (`!logicalConnection.isPhysicallyConnected()`). If so, acquire and initialize its physical connection (`con.getMetaData()`) from the data source pool beforehand. This ensures the outer session holds its own independent connection and does not borrow the inner transaction's thread-bound connection during lazy loading.
2. In `GrailsHibernateTemplate.convertHibernateAccessException`, fall back to `SessionFactoryUtils.convertHibernateAccessException(ex)` if `convertJdbcAccessException` returns `null`.
3. In `GrailsHibernateTemplate.doExecute`, fall back to `new HibernateSystemException(ex)` if `convertHibernateAccessException` returns `null`.
## Tests
- `WithNewSessionLazyLoadSpec`: reproduces the scenario where a lazy association on an outer request-bound session entity is loaded inside a nested `withNewSession { withTransaction { ... } }`, and verifies that the outer session remains open and usable for subsequent queries and updates.
- `GrailsHibernateTemplateSpec`: added test verifying fallback to `SessionFactoryUtils` when JDBC exception translation returns null.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4385378969
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +221,36 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // con.getAutoCommit() is used because TransactionAwareDataSourceProxy only obtains a target
+ // connection on its first real method call, binding the proxy to a target connection.
+ // Unlike getMetaData(), getAutoCommit() allows LazyConnectionDataSourceProxy to answer
+ // without eagerly checking out a connection from the pool when nothing is lazy-loaded.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getAutoCommit();
Review Comment:
This registers a JDBC `ConnectionSynchronization` and not the session synchronization. When the scope ends, Spring closes the connection and the outer session still holds it. The next query fails with connection is closed. The new tests query the outer session before leaving the scope, which hides this.
Disconnect the outer session with that connection. Please leave the scope without touching the outer session, then query it.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6458065845
Pushed commit `0d92e40c7d` addressing both non-blocking notes:
- Added clarifying comments to `GrailsHibernateTemplate.java` explaining the necessity of registering the synchronization at `SESSION_SYNCHRONIZATION_ORDER` to prevent Spring's JDBC `ConnectionSynchronization` from closing outer session connections when no in-scope query registers a session synchronization.
- Removed the in-scope outer query from the `PROPAGATION_SUPPORTS` lazy-load test in `WithNewSessionLazyLoadSpec.groovy` so the feature directly tests the scenario without registering an in-scope session synchronization.
@jdaugherty if everything looks good, please approve when you have a moment.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub) via notifications" <no...@grails.apache.org>.
borinquenkid commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6539366329
Rebased onto `8.0.x` and squashed to commit `bb56460f51`, bringing Hibernate 7 in full alignment with the strategy and lifecycle refinements established in #16784:
1. **Lazy connection gating (`dataSource.lazy=false`)**:
- Priming during `prepare()` is gated strictly by `lazyConnection == true`, ensuring idle outer sessions on non-lazy connection pools do not eagerly check out connections.
- `wasPhysicallyConnectedAtPrepare` is tracked in `AbstractOuterSessionLifecycleStrategy`. In `restore()`, physical connections are disconnected via `disconnectIfConnected()` when `!wasPhysicallyConnectedAtPrepare && (!lazyConnection || !previousActiveSynchronization)`, returning checked-out connections to the pool and ensuring inner transactions under `PROPAGATION_SUPPORTS` do not leave connections held by the outer session.
2. **Closed outer session handling**:
- `OuterSessionLifecycleStrategy.from(...)` checks `previousHolder != null` rather than `session.isOpen()`, routing closed bound sessions through `DefaultOuterSessionStrategy` so that outer session holders are cleanly suspended before `withNewSession` and restored upon scope exit.
3. **Exception translation and execution delegation**:
- New session execution delegates through `execute(callable::call)`, ensuring flush modes (`FLUSH_EAGER`), `osivReadOnly`, and translation of Hibernate exceptions to Spring `DataAccessException` (`HibernateSystemException`) are preserved.
4. **Surface cleanups**:
- Removed unused `isLazyConnection()`, `transactionAware`, and 4-argument constructors / factory overloads.
- Guarded DataSource lookups in transaction resource operations against null references.
- Added test coverage in `WithNewSessionNonLazyPoolSpec`, `WithNewSessionLazyLoadSpec`, and `GrailsHibernateTemplateSpec`.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "codecov[bot] (via GitHub) via notifications" <no...@grails.apache.org>.
codecov[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6594879068
## [Codecov](https://app.codecov.io/gh/apache/grails-core/pull/16763?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.0000%. Comparing base ([`8803d54`](https://app.codecov.io/gh/apache/grails-core/commit/8803d54c23f742d11efb3e01eeee86489bb4ba95?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b715c03`](https://app.codecov.io/gh/apache/grails-core/commit/b715c0390b97b7f48ce210de5a4cc4d1d7689657?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on 8.0.x.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/grails-core/pull/16763?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## 8.0.x #16763 +/- ##
==============================
==============================
```
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6452526470
Updated in commit cba1b7f74e:
1. Registered a `TransactionSynchronization` at `SessionFactoryUtils.SESSION_SYNCHRONIZATION_ORDER` alongside the `con.getAutoCommit()` physical connection acquisition in `GrailsHibernateTemplate.executeWithNewSession`. If no in-scope query registers a Hibernate session synchronization, the logical connection disconnects before Spring's `ConnectionSynchronization` closes the physical connection at transaction completion.
2. Added test coverage in `WithNewSessionLazyLoadSpec` verifying that a request-bound session remains open and usable following a `PROPAGATION_SUPPORTS` transaction wrapping a nested `withNewSession` without any query in the outer scope.
Also opened #16783 to address the same issue in `grails-data-hibernate5` on `7.0.x`.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6458977727
## β
All tests passed β
π·οΈ Commit: 0d92e40c7de7ff0fb89d6bd5408d352880526049
βΆοΈ Tests: 92107 executed
βͺοΈ Checks: 96/96 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6514729984
## β
All tests passed β
π·οΈ Commit: c9eeb390f8ae5838f1d581e05b69040cd2724e76
βΆοΈ Tests: 92493 executed
βͺοΈ Checks: 96/96 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub) via notifications" <no...@grails.apache.org>.
borinquenkid commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6594873609
@jdaugherty @jamesfredley head b715c0390b: `Hibernate7RefreshLockSpec` failed on the previous head because `executeWithNewSession` went through `getCurrentSession()`, which synchronized the new holder with the surrounding transaction so a named connection's `withTransaction` opened a second session. The callback now runs on the session `withNewSession` opened. The Hibernate7, Hibernate5, MongoDB and Neo4j functional jobs pass on this head. The jobs still failing are the Forge and Build Grails-Core jobs that also fail on 8.0.x, and the grails-openapi `PersistentEntitySchemaSpec` failure in Build Grails, which does not touch this code and also fails on #16583. Could you take another look?
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4409717857
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +232,60 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // This workaround is only taken when the connection provider uses TransactionAwareDataSourceProxy
+ // backed by LazyConnectionDataSourceProxy: con.getAutoCommit() allows LazyConnectionDataSourceProxy
+ // to bind the proxy without eagerly checking out a connection from the pool. When lazy wrapping is off
+ // or the data source is non-transaction-aware, acquiring the connection eagerly would check out a pooled
+ // connection unnecessarily and cause a single-connection pool to time out.
+ if (this.transactionAware && this.lazyConnection &&
Review Comment:
This priming runs only when lazy wrapping is on. With dataSource.lazy=false and transactionAware still true, a disconnected outer session is not given its own connection. A lazy load inside withNewSession { withTransaction { ... } } then takes the inner transaction's connection, and the outer session keeps that connection after the inner transaction closes it. The idle outer session must not check out a pooled connection, and a real use of that session must not share the inner transaction's connection.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +232,60 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // This workaround is only taken when the connection provider uses TransactionAwareDataSourceProxy
+ // backed by LazyConnectionDataSourceProxy: con.getAutoCommit() allows LazyConnectionDataSourceProxy
+ // to bind the proxy without eagerly checking out a connection from the pool. When lazy wrapping is off
+ // or the data source is non-transaction-aware, acquiring the connection eagerly would check out a pooled
+ // connection unnecessarily and cause a single-connection pool to time out.
+ if (this.transactionAware && this.lazyConnection &&
+ previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getAutoCommit();
+ } catch (SQLException e) {
+ LOG.warn("Could not initialize physical connection: {}", e.getMessage(), e);
+ }
+ if (txResources.isSynchronizationActive() && !txResources.isActualTransactionActive()) {
+ // When nothing in the scope uses the outer session in a non-actual-transaction scope
+ // (such as PROPAGATION_SUPPORTS with synchronization), no session synchronization
+ // is registered, and the JDBC ConnectionSynchronization would close the connection
+ // the outer session still holds. Registering at SESSION_SYNCHRONIZATION_ORDER ensures
+ // the session is disconnected before Spring's ConnectionSynchronization runs.
+ // We strictly restrict this to scopes that are NOT an actual transaction to prevent
+ // manualDisconnect() from severing active write-bearing transactions before doCommit.
+ txResources.registerSynchronization(new TransactionSynchronization() {
+ @Override
+ public int getOrder() {
+ return SessionFactoryUtils.SESSION_SYNCHRONIZATION_ORDER;
+ }
+
+ @Override
+ public void beforeCompletion() {
Review Comment:
Inside a SUPPORTS scope, this beforeCompletion does not run when a REQUIRED transaction suspends that synchronization. The REQUIRED transaction reuses the connection this scope primed and closes it, so a later query on the outer session fails with a closed connection. Disconnect the primed session when the synchronization is suspended.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6511743660
## π¨ All tests passed but jobs failed π¨
### Failed Jobs without Test Failures
β [CI / Spring Security Tests \(Java 21\)](https://github.com/apache/grails-core/actions/runs/40966382904/job/123783773204?pr=16763)
π·οΈ Commit: 1b4eaafcd49c66bb2a51cfce743c5bcc731c5e32
βΆοΈ Tests: 90956 executed
π‘ Checks: 82/92 completed
### Rerun Controls
Click the checkbox to trigger a rerun:
π² **Rerun jobs**
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub) via notifications" <no...@grails.apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6537598337
## π¨ TestLens detected 1 failed test π¨
### Test Summary
> [!TIP]
> Inspect all details with the [TestLens CLI](https://testlens.app/docs/features/cli): `testlens pr 16763`
#### [Groovy Snapshot Canary Build / Build Grails \(shard 2\)](https://github.com/apache/grails-core/actions/runs/41157602385/job/124408216877?pr=16763) > :grails-data-hibernate7-core:test
| Test | Runs | Flakiness |
|---|---|--:|
| Hibernate7RefreshLockSpec > refresh\(lock: true\) uses the named connection rather than the default database | β | 0% π’ |
π·οΈ Commit: 67880b478c06fe72b526db063706cc19e38e54f7
βΆοΈ Tests: 27550 executed
π‘ Checks: 37/90 completed
### Test Failures
<details><summary><strong>Hibernate7RefreshLockSpec > refresh(lock: true) uses the named connection rather than the default database</strong> (:grails-data-hibernate7-core:test in <a href="https://github.com/apache/grails-core/actions/runs/41157602385/job/124408216877?pr=16763">Groovy Snapshot Canary Build / Build Grails (shard 2)</a>)</summary>
```
Condition not satisfied:
session.transaction.isActive()
| | |
| | false
| <org.hibernate.engine.transaction.internal.TransactionImpl@35ca3810 transactionCoordinator=inaccessible jpaCompliance=inaccessible session=inaccessible transactionDriverControl=inaccessible>
SessionImpl(14008208<open>)
at grails.gorm.tests.Hibernate7RefreshLockSpec.$spock_feature_2_18_closure43$_closure142(Hibernate7RefreshLockSpec.groovy:655)
at grails.gorm.transactions.GrailsTransactionTemplate$2.doInTransaction(GrailsTransactionTemplate.groovy:139)
at org.springframework.transaction.support.TransactionTemplate.execute(TransactionTemplate.java:137)
at grails.gorm.transactions.GrailsTransactionTemplate.doExecute(GrailsTransactionTemplate.groovy:136)
at grails.gorm.transactions.GrailsTransactionTemplate.execute_closure2(GrailsTransactionTemplate.groovy:131)
at grails.gorm.transactions.GrailsTransactionTemplate.inConnectionScope(GrailsTransactionTemplate.groovy:96)
at grails.gorm.transactions.GrailsTransactionTemplate.execute(GrailsTransactionTemplate.groovy:131)
at org.grails.datastore.gorm.GormStaticApi.withTransaction_closure24(GormStaticApi.groovy:1003)
at org.grails.datastore.gorm.GormStaticApi.withTransaction_closure24(GormStaticApi.groovy)
at org.grails.datastore.gorm.GormRegistry.runInScope(GormRegistry.groovy:547)
at org.grails.datastore.gorm.GormRegistry.withConnectionScope(GormRegistry.groovy:518)
at org.grails.datastore.gorm.GormStaticApi.inConnectionScope(GormStaticApi.groovy:977)
at org.grails.datastore.gorm.GormStaticApi.withTransaction(GormStaticApi.groovy:1002)
at grails.gorm.tests.Hibernate7RefreshLockSpec.refresh(lock: true) uses the named connection rather than the default database_closure43(Hibernate7RefreshLockSpec.groovy:654)
at org.grails.orm.hibernate.GrailsHibernateTemplate.doExecute(GrailsHibernateTemplate.java:369)
at org.grails.orm.hibernate.GrailsHibernateTemplate.execute(GrailsHibernateTemplate.java:308)
at org.grails.orm.hibernate.GrailsHibernateTemplate.executeWithNewSession(GrailsHibernateTemplate.java:242)
at org.grails.orm.hibernate.HibernateDatastore.withNewSession(HibernateDatastore.java:1052)
at org.grails.orm.hibernate.HibernateGormStaticApi.withNewSession_closure1(HibernateGormStaticApi.groovy:165)
at org.grails.datastore.gorm.GormRegistry.runInScope(GormRegistry.groovy:547)
at org.grails.datastore.gorm.GormRegistry.withConnectionScope(GormRegistry.groovy:518)
at org.grails.datastore.gorm.GormStaticApi.inConnectionScope(GormStaticApi.groovy:977)
at org.grails.orm.hibernate.HibernateGormStaticApi.withNewSession(HibernateGormStaticApi.groovy:159)
at grails.gorm.tests.Hibernate7RefreshLockSpec.refresh(lock: true) uses the named connection rather than the default database(Hibernate7RefreshLockSpec.groovy:653)
```
</details>
### Rerun Controls
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
π² Hibernate7RefreshLockSpec > refresh\(lock: true\) uses the named connection rather than the default database <!-- uniqueId=[engine:spock]/[spec:grails.gorm.tests.Hibernate7RefreshLockSpec]/[feature:$spock_feature_2_18] -->
Reuse successful test results:
π² β»οΈ Only rerun the tests that failed or were muted before
Click the checkbox to trigger a rerun:
π² **Rerun jobs**
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4396226976
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +221,55 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // con.getAutoCommit() is used because TransactionAwareDataSourceProxy only obtains a target
+ // connection on its first real method call, binding the proxy to a target connection.
+ // Unlike getMetaData(), getAutoCommit() allows LazyConnectionDataSourceProxy to answer
+ // without eagerly checking out a connection from the pool when nothing is lazy-loaded.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getAutoCommit();
+ } catch (SQLException e) {
+ LOG.warn("Could not initialize physical connection: {}", e.getMessage(), e);
+ }
+ if (txResources.isSynchronizationActive()) {
+ // When nothing in the scope uses the outer session, no session synchronization
+ // is registered, and the JDBC ConnectionSynchronization would close the connection
+ // the outer session still holds. Registering at SESSION_SYNCHRONIZATION_ORDER ensures
+ // the session is disconnected before Spring's ConnectionSynchronization runs.
+ txResources.registerSynchronization(new TransactionSynchronization() {
+ @Override
+ public int getOrder() {
+ return SessionFactoryUtils.SESSION_SYNCHRONIZATION_ORDER;
+ }
+
+ @Override
+ public void beforeCompletion() {
+ if (logicalConnection.isOpen() && logicalConnection.isPhysicallyConnected()) {
+ logicalConnection.manualDisconnect();
Review Comment:
With hibernate.connection.provider_disables_autocommit=true, a non-transaction-aware data source, and pool autocommit off, an insert after executeWithNewSession {} commits and leaves zero rows. The base keeps the row. beforeCompletion calls manualDisconnect before HibernateTransactionManager.doCommit. Restrict this synchronization to scopes that are not an actual transaction, and keep the SUPPORTS cleanup.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +221,55 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // con.getAutoCommit() is used because TransactionAwareDataSourceProxy only obtains a target
+ // connection on its first real method call, binding the proxy to a target connection.
+ // Unlike getMetaData(), getAutoCommit() allows LazyConnectionDataSourceProxy to answer
+ // without eagerly checking out a connection from the pool when nothing is lazy-loaded.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
Review Comment:
When lazy connection wrapping is off, getPhysicalConnection checks out a pooled connection even if the outer session is never used. A one-connection pool then times out on withNewSession. The base query succeeds. Only take this connection when the transaction-aware proxy workaround needs it.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6513641722
Rebased onto `8.0.x` and squashed.
We have refactored outer session lifecycle handling in `executeWithNewSession` to use dedicated `OuterSessionLifecycleStrategy` implementations matching the isolation requirements of each scope:
- **`ActiveTransactionOuterSessionStrategy`**: When an outer write transaction is active, the outer session is suspended and rebound symmetrically without disconnecting or unbinding holders early, preserving active transaction state and flushed changes.
- **`NonTransactionalSynchronizationOuterSessionStrategy`**: In non-transactional scopes under active Spring synchronization (e.g. `PROPAGATION_SUPPORTS` or OSIV), the outer session eagerly primes its connection so that lazy loads inside the new session's transaction do not borrow the inner connection, unbinds the session holder during new session execution, and registers synchronization to disconnect the outer session both on suspension (when a nested `REQUIRED` transaction begins) and before completion prior to Spring JDBC connection cleanup.
- **`DefaultOuterSessionStrategy`**: Handles outer sessions where synchronization is inactive by priming disconnected sessions and unbinding before the new session runs, restoring it upon completion.
- **`TransactionResources`**: Abstracts Spring's `TransactionSynchronizationManager` interactions for full decoupling and testability.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4420902784
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/AbstractOuterSessionLifecycleStrategy.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.orm.hibernate;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.List;
+
+import javax.sql.DataSource;
+
+import org.hibernate.Session;
+import org.hibernate.SessionFactory;
+import org.hibernate.engine.jdbc.spi.JdbcCoordinator;
+import org.hibernate.engine.spi.SessionImplementor;
+import org.hibernate.resource.jdbc.spi.LogicalConnectionImplementor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.springframework.jdbc.datasource.ConnectionHolder;
+import org.springframework.transaction.support.TransactionSynchronization;
+
+import org.grails.orm.hibernate.support.hibernate7.SessionHolder;
+import org.grails.orm.hibernate.support.hibernate7.TransactionResources;
+
+/**
+ * Base strategy handling symmetric capture, unbinding, and restoration of session and connection holders
+ * as well as Spring transaction synchronizations.
+ */
+abstract class AbstractOuterSessionLifecycleStrategy implements OuterSessionLifecycleStrategy {
+
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractOuterSessionLifecycleStrategy.class);
+
+ protected final SessionFactory sessionFactory;
+ protected final DataSource dataSource;
+ protected final TransactionResources txResources;
+ protected final SessionHolder previousHolder;
+
+ protected ConnectionHolder previousConnectionHolder;
+ protected boolean previousActiveSynchronization;
+ protected List<TransactionSynchronization> transactionSynchronizations;
+
+ AbstractOuterSessionLifecycleStrategy(
+ SessionFactory sessionFactory,
+ DataSource dataSource,
+ TransactionResources txResources,
+ SessionHolder previousHolder) {
+ this.sessionFactory = sessionFactory;
+ this.dataSource = dataSource;
+ this.txResources = txResources;
+ this.previousHolder = previousHolder;
+ }
+
+ protected void primeIfDisconnected() {
+ if (previousHolder == null || previousHolder.getSession() == null || !previousHolder.getSession().isOpen()) {
+ return;
+ }
+
+ Session previousSession = previousHolder.getSession();
+ if (!(previousSession instanceof SessionImplementor sessionImpl)) {
+ return;
+ }
+
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator == null) {
+ return;
+ }
+
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection == null || logicalConnection.isPhysicallyConnected()) {
+ return;
+ }
+
+ Connection con = logicalConnection.getPhysicalConnection();
Review Comment:
With `transactionAware=false`, `lazy=false`, a one-connection pool, and an open disconnected request session, `withNewSession { withTransaction { /* query */ } }` checks out the only connection for the unused outer session. The inner query then waits until the pool times out. The same priming in `NonTransactionalSynchronizationOuterSessionStrategy.prepare()` does this too. Prime a transaction-aware source, and skip a source that cannot lend its connection.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/OuterSessionLifecycleStrategy.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.orm.hibernate;
+
+import javax.sql.DataSource;
+
+import org.hibernate.SessionFactory;
+
+import org.grails.orm.hibernate.support.hibernate7.SessionHolder;
+import org.grails.orm.hibernate.support.hibernate7.TransactionResources;
+
+/**
+ * Strategy interface governing outer session lifecycle preparation, suspension, and restoration
+ * across nested {@code withNewSession} invocations.
+ */
+interface OuterSessionLifecycleStrategy {
+
+ /**
+ * Prepares the outer session before unbinding and opening the new session.
+ */
+ void prepare();
+
+ /**
+ * Suspends any active Spring synchronizations and unbinds outer session and connection holders.
+ */
+ void suspendAndUnbind();
+
+ /**
+ * Restores previously suspended synchronizations and rebinds outer session and connection holders.
+ */
+ void restore();
+
+ /**
+ * Factory method creating the appropriate strategy based on the outer session, connection, and transaction context.
+ *
+ * @param previousHolder the session holder bound to the outer scope, or null
+ * @param sessionFactory the current Hibernate SessionFactory
+ * @param dataSource the underlying DataSource
+ * @param txResources the transaction synchronization and resource manager
+ * @return the appropriate {@link OuterSessionLifecycleStrategy}
+ */
+ static OuterSessionLifecycleStrategy from(
+ SessionHolder previousHolder,
+ SessionFactory sessionFactory,
+ DataSource dataSource,
+ TransactionResources txResources) {
+
+ boolean hasOpenSession = previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen();
+ boolean hasConnectionHolder = txResources.getResource(dataSource) != null;
+ boolean hasSynchronization = txResources.isSynchronizationActive();
+
+ if (!hasOpenSession && !hasConnectionHolder && !hasSynchronization) {
Review Comment:
A closed session still bound, with no connection holder and no synchronization, selects the strategy that never unbinds that holder. `executeWithNewSession()` then throws `IllegalStateException` because a value is already bound, and the callback does not run. Use the no-op strategy only when no holder is bound, and unbind a holder whose session is closed.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4377789786
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -236,6 +239,30 @@ public <T> T executeWithNewSession(final Closure<T> callable) {
txResources.initSynchronization();
}
+ // If the outer session is disconnected, acquire its physical connection now before
+ // unbinding and entering the new session scope. Otherwise, lazy loading on outer session
+ // entities inside withNewSession { withTransaction { ... } } would borrow the inner
+ // transaction's connection and be left with a closed connection when the inner transaction commits.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getMetaData();
Review Comment:
This call is what makes the fix work, and the comment above doesn't say why. `getPhysicalConnection()` returns the `TransactionAwareDataSourceProxy` handle, and the proxy only takes a target connection on its first real method call. `getMetaData()` is that call. When I replaced it with a call the proxy answers itself (`hashCode()`), `WithNewSessionLazyLoadSpec` failed again with `PooledConnection has already been closed`. Please say that in the comment, so the call doesn't get removed as dead code.
`getMetaData()` also makes the default `LazyConnectionDataSourceProxy` (`dataSource.lazy: true`) check out a pooled connection, and the request session holds it until it closes. That happens on every `withNewSession` while the request session is disconnected. It includes a request whose first database work is the `withNewSession` and whose session has nothing to lazy-load. After `withNewSession { WnsllAuthor.count() }` on an unused request session, the Tomcat pool reports 1 active connection on this branch and 0 on 8.0.x. `con.getAutoCommit()` binds the proxy's target the same way, but the lazy proxy answers it without fetching from the pool. With that change the spec still passes, and the pool stays at 0 until the request session runs a statement. (With `dataSource.lazy: false`, both calls check out a connection.)
The `SQLException` is logged at debug. If this call fails, the original bug comes back with nothing in the log at the default level. Logging it at warn would make that visible.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -680,10 +708,12 @@ protected void flushIfNecessary(Session session, boolean existingTransaction) th
@SuppressWarnings("ConstantConditions")
protected DataAccessException convertHibernateAccessException(HibernateException ex) {
if (ex instanceof JDBCException) {
- return convertJdbcAccessException((JDBCException) ex, jdbcExceptionTranslator);
+ DataAccessException dae = convertJdbcAccessException((JDBCException) ex, jdbcExceptionTranslator);
+ return dae != null ? dae : SessionFactoryUtils.convertHibernateAccessException(ex);
}
if (GenericJDBCException.class.equals(ex.getClass())) {
Review Comment:
This branch can't run. `GenericJDBCException` extends `JDBCException`, so the `instanceof JDBCException` check above catches it first. The new test passes a `GenericJDBCException`, but it goes through the first branch. Could you delete this branch instead of adding the fallback to it?
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -410,7 +437,8 @@ protected <T> T doExecute(HibernateCallback<T> action, boolean enforceNativeSess
flushIfNecessary(session, existingTransaction);
return result;
} catch (HibernateException ex) {
- throw convertHibernateAccessException(ex);
+ DataAccessException dae = convertHibernateAccessException(ex);
+ throw (dae != null ? dae : new HibernateSystemException(ex));
Review Comment:
After the change to `convertHibernateAccessException`, it can't return null. Both branches fall back to `SessionFactoryUtils.convertHibernateAccessException`, which ends with `new HibernateSystemException(ex)`, and nothing in the repository overrides the method. So this second fallback never runs, and no test covers it. I'd drop it.
This path does need a test, though, because the issue's `NullPointerException` came from here. With the fix in place, the new spec no longer reaches it, and the new `GrailsHibernateTemplateSpec` test calls `convertHibernateAccessException` directly. Next to `doExecute wraps HibernateException as DataAccessException`, something like this:
```groovy
when:
template.execute { sess -> throw new GenericJDBCException('msg', new SQLException('untranslatable')) }
then:
thrown(HibernateJdbcException)
```
On 8.0.x that throws the `NullPointerException`. On this branch it throws `HibernateJdbcException`.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -236,6 +239,30 @@ public <T> T executeWithNewSession(final Closure<T> callable) {
txResources.initSynchronization();
}
+ // If the outer session is disconnected, acquire its physical connection now before
+ // unbinding and entering the new session scope. Otherwise, lazy loading on outer session
+ // entities inside withNewSession { withTransaction { ... } } would borrow the inner
+ // transaction's connection and be left with a closed connection when the inner transaction commits.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
Review Comment:
Reproduced. It is also a regression. This works on 8.0.x and fails on this branch, with no lazy load involved:
```groovy
WnsllAuthor.withTransaction { WnsllAuthor.count() } // disconnects the request session
WnsllBook.withTransaction(propagationBehavior: TransactionDefinition.PROPAGATION_SUPPORTS, readOnly: true) {
WnsllBook.withNewSession {
WnsllBook.withTransaction { new WnsllAuthor(name: 'Tenar').save(flush: true) }
}
WnsllBook.count() // 8.0.x: 1. This branch: HibernateJdbcException, PooledConnection has already been closed
}
```
`getMetaData()` runs after the synchronization is re-initialised, so `DataSourceUtils` binds a `ConnectionHolder` for the request session's connection. `previousConnectionHolder` was already read as null, so that holder stays bound through the new-session scope. The `finally` block's `unbindResourceIfPossible(dataSource)` then finds it, and `releaseConnection` closes the connection the request session is still holding. In my runs the nested `withTransaction` itself got through, because its `suspend` unbinds that holder before `doBegin` binds its own. What fails is the next statement on the request session.
Moving the acquisition above the `previousConnectionHolder` and `getSynchronizations()` reads fixes it. With only that change, three things pass: the case above, the same case with `book.author.name` read inside the inner transaction (which throws the `NullPointerException` on 8.0.x), and `WithNewSessionLazyLoadSpec`.
For the spec, put `readOnly: true` on the SUPPORTS scope. A read-write SUPPORTS scope that used the request session ends with `TransactionRequiredException: No active transaction`, from the flush at commit. That happens on 8.0.x too, and it would hide the result.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4390857333
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +221,36 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // con.getAutoCommit() is used because TransactionAwareDataSourceProxy only obtains a target
+ // connection on its first real method call, binding the proxy to a target connection.
+ // Unlike getMetaData(), getAutoCommit() allows LazyConnectionDataSourceProxy to answer
+ // without eagerly checking out a connection from the pool when nothing is lazy-loaded.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getAutoCommit();
Review Comment:
Reproduced. It is also a regression. This works on 8.0.x and fails on this branch:
```groovy
WnsllAuthor.withTransaction { WnsllAuthor.count() } // disconnects the request session
WnsllBook.withTransaction(propagationBehavior: TransactionDefinition.PROPAGATION_SUPPORTS, readOnly: true) {
WnsllBook.withNewSession { WnsllAuthor.count() }
}
WnsllBook.count() // 8.0.x: 1. This branch: HibernateJdbcException, connection is closed
```
It fails the same way with a `withTransaction` inside the `withNewSession`, and with a read-write `SUPPORTS` scope.
The issue's own scenario is also still broken under `SUPPORTS`. The third new feature, without its `assert WnsllBook.count() == 1` inside the scope, fails after the scope with `PooledConnection has already been closed.` On 8.0.x it threw the `NullPointerException`. That in-scope query registers the session synchronization, which is why all three `SUPPORTS` features pass.
Registering a synchronization next to the `getAutoCommit()` call fixes all of these. It runs before the `ConnectionSynchronization` and disconnects the session, as `SpringSessionSynchronization.beforeCompletion` does. Because the block now runs before the synchronizations are saved, the `finally` block restores it with the others:
```java
if (txResources.isSynchronizationActive()) {
txResources.registerSynchronization(new TransactionSynchronization() {
@Override
public int getOrder() {
return SessionFactoryUtils.SESSION_SYNCHRONIZATION_ORDER;
}
@Override
public void beforeCompletion() {
if (logicalConnection.isOpen() && logicalConnection.isPhysicallyConnected()) {
logicalConnection.manualDisconnect();
}
}
});
}
```
With that, the cases above, `WithNewSessionLazyLoadSpec`, and `GrailsHibernateTemplateSpec` pass.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4394651504
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +221,51 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // con.getAutoCommit() is used because TransactionAwareDataSourceProxy only obtains a target
+ // connection on its first real method call, binding the proxy to a target connection.
+ // Unlike getMetaData(), getAutoCommit() allows LazyConnectionDataSourceProxy to answer
+ // without eagerly checking out a connection from the pool when nothing is lazy-loaded.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getAutoCommit();
+ } catch (SQLException e) {
+ LOG.warn("Could not initialize physical connection: {}", e.getMessage(), e);
+ }
+ if (txResources.isSynchronizationActive()) {
Review Comment:
Non-blocking: the comment above explains the acquisition but not this registration, and without a reason it reads as redundant with the session synchronization. One line would keep it: when nothing in the scope uses the outer session, no session synchronization is registered, and the JDBC `ConnectionSynchronization` would close the connection the outer session still holds.
##########
grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/WithNewSessionLazyLoadSpec.groovy:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.gorm.tests
+
+import grails.gorm.annotation.Entity
+import org.apache.grails.data.hibernate7.core.GrailsDataHibernate7TckManager
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import org.grails.orm.hibernate.proxy.HibernateProxyHandler
+import org.grails.orm.hibernate.support.hibernate7.SessionHolder
+import org.springframework.transaction.TransactionDefinition
+import org.springframework.transaction.support.TransactionSynchronizationManager
+import spock.lang.Issue
+
+/**
+ * Verifies that the request-bound session remains valid after a lazy load
+ * occurs within a nested withNewSession { withTransaction { ... } } block.
+ */
+@Issue('https://github.com/apache/grails-core/issues/16571')
+class WithNewSessionLazyLoadSpec extends GrailsDataTckSpec<GrailsDataHibernate7TckManager> {
+
+ void setupSpec() {
+ manager.isTransactional = false
+ manager.registerDomainClasses(WnsllBook, WnsllAuthor)
+ }
+
+ void cleanup() {
+ WnsllAuthor.withTransaction {
+ WnsllBook.where {}.deleteAll()
+ WnsllAuthor.where {}.deleteAll()
+ }
+ manager.sessionFactory.currentSession.clear()
+ }
+
+ void "request-bound session remains usable after lazy load inside withNewSession with inner transaction"() {
+ given: "a seeded author and book"
+ WnsllAuthor.withTransaction {
+ WnsllAuthor author = new WnsllAuthor(name: 'Stephen King').save()
+ new WnsllBook(title: 'The Stand', author: author).save()
+ }
+ manager.sessionFactory.currentSession.clear()
+
+ and: "an active request-bound session in TSM"
+ manager.sessionFactory.currentSession
+ SessionHolder requestSessionHolder = TransactionSynchronizationManager.getResource(manager.sessionFactory)
+
+ when: "loading an entity in the request session without a transaction"
+ WnsllBook book = WnsllBook.first()
+ HibernateProxyHandler proxyHandler = new HibernateProxyHandler()
+ assert !proxyHandler.isInitialized(book, 'author')
+
+ and: "running and ending a transaction on the request session"
+ WnsllAuthor.withTransaction { WnsllAuthor.count() }
+
+ and: "calling withNewSession with an inner transaction that lazy-loads an association on the request session entity"
+ WnsllBook.withNewSession {
+ WnsllBook.withTransaction {
+ assert book.author.name == 'Stephen King'
+ new WnsllAuthor(name: 'Tenar').save()
+ }
+ }
+
+ then: "the request-bound session remains open and usable"
+ requestSessionHolder.session.isOpen()
+ WnsllBook.count() == 1
+ WnsllAuthor.count() == 2
+ book.refresh()
+ book.title == 'The Stand'
+ book.author.name == 'Stephen King'
+ }
+
+ void "request-bound session remains usable after nested withNewSession inside SUPPORTS transaction"() {
+ given: "a seeded author and book"
+ WnsllAuthor.withTransaction {
+ WnsllAuthor author = new WnsllAuthor(name: 'Ursula').save()
+ new WnsllBook(title: 'Earthsea', author: author).save()
+ }
+ manager.sessionFactory.currentSession.clear()
+
+ and: "an active request-bound session in TSM"
+ manager.sessionFactory.currentSession
+ SessionHolder requestSessionHolder = TransactionSynchronizationManager.getResource(manager.sessionFactory)
+
+ when: "disconnecting the request session via an initial transaction"
+ WnsllAuthor.withTransaction { WnsllAuthor.count() }
+
+ and: "running a SUPPORTS read-only transaction with a nested withNewSession and inner transaction"
+ WnsllBook.withTransaction(propagationBehavior: TransactionDefinition.PROPAGATION_SUPPORTS, readOnly: true) {
+ WnsllBook.withNewSession {
+ WnsllBook.withTransaction { new WnsllAuthor(name: 'Tenar').save(flush: true) }
+ }
+ assert WnsllBook.count() == 1
+ }
+
+ then: "the request-bound session remains open and usable"
+ requestSessionHolder.session.isOpen()
+ WnsllBook.count() == 1
+ WnsllAuthor.count() == 2
+ }
+
+ void "request-bound session remains usable after nested withNewSession inside SUPPORTS transaction without in-scope query"() {
+ given: "a seeded author and book"
+ WnsllAuthor.withTransaction {
+ WnsllAuthor author = new WnsllAuthor(name: 'Ursula').save()
+ new WnsllBook(title: 'Earthsea', author: author).save()
+ }
+ manager.sessionFactory.currentSession.clear()
+
+ and: "an active request-bound session in TSM"
+ manager.sessionFactory.currentSession
+ SessionHolder requestSessionHolder = TransactionSynchronizationManager.getResource(manager.sessionFactory)
+
+ when: "disconnecting the request session via an initial transaction"
+ WnsllAuthor.withTransaction { WnsllAuthor.count() }
+
+ and: "running a SUPPORTS read-only transaction with a nested withNewSession without in-scope query"
+ WnsllBook.withTransaction(propagationBehavior: TransactionDefinition.PROPAGATION_SUPPORTS, readOnly: true) {
+ WnsllBook.withNewSession { WnsllAuthor.count() }
+ }
+
+ then: "the request-bound session remains open and usable after the scope completes"
+ requestSessionHolder.session.isOpen()
+ WnsllBook.count() == 1
+ }
+
+ void "request-bound session remains usable after lazy load inside withNewSession under SUPPORTS transaction"() {
+ given: "a seeded author and book"
+ WnsllAuthor.withTransaction {
+ WnsllAuthor author = new WnsllAuthor(name: 'Ursula').save()
+ new WnsllBook(title: 'Earthsea', author: author).save()
+ }
+ manager.sessionFactory.currentSession.clear()
+
+ and: "an active request-bound session in TSM"
+ manager.sessionFactory.currentSession
+ SessionHolder requestSessionHolder = TransactionSynchronizationManager.getResource(manager.sessionFactory)
+
+ when: "loading an entity in the request session without a transaction"
+ WnsllBook book = WnsllBook.first()
+ HibernateProxyHandler proxyHandler = new HibernateProxyHandler()
+ assert !proxyHandler.isInitialized(book, 'author')
+
+ and: "disconnecting the request session via an initial transaction"
+ WnsllAuthor.withTransaction { WnsllAuthor.count() }
+
+ and: "calling withNewSession under a SUPPORTS scope with an inner transaction that lazy-loads"
+ WnsllBook.withTransaction(propagationBehavior: TransactionDefinition.PROPAGATION_SUPPORTS, readOnly: true) {
+ WnsllBook.withNewSession {
+ WnsllBook.withTransaction {
+ assert book.author.name == 'Ursula'
+ new WnsllAuthor(name: 'Tenar').save(flush: true)
+ }
+ }
+ assert WnsllBook.count() == 1
Review Comment:
Non-blocking: this query registers the session synchronization, so this feature passes without the new synchronization too. Without it, the feature covers the issue's scenario under `SUPPORTS`, which is the case that needed it.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub) via notifications" <no...@grails.apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6593117084
## π¨ TestLens detected 4 failed tests π¨
### Test Summary
> [!TIP]
> Inspect all details with the [TestLens CLI](https://testlens.app/docs/features/cli): `testlens pr 16763`
#### [CI / Build Grails-Core \(Windows JDK 25 shard 1\)](https://github.com/apache/grails-core/actions/runs/41558393825/job/125662826977?pr=16763) > :grails-openapi:test
| Test | Runs | Flakiness |
|---|---|--:|
| PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context | β | 0% π’ |
#### [CI / Build Grails-Core Rerunning all Tasks \(Ubuntu JDK 21 shard 1\)](https://github.com/apache/grails-core/actions/runs/41558393825/job/125662827029?pr=16763) > :grails-openapi:test
| Test | Runs | Flakiness |
|---|---|--:|
| PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context | β | 0% π’ |
#### [Groovy Snapshot Canary Build / Build Grails \(shard 1\)](https://github.com/apache/grails-core/actions/runs/41558393873/job/125663790601?pr=16763) > :grails-openapi:test
| Test | Runs | Flakiness |
|---|---|--:|
| PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context | β | 0% π’ |
#### [Groovy Snapshot Canary Build / Build Grails \(shard 2\)](https://github.com/apache/grails-core/actions/runs/41558393873/job/125663790395?pr=16763) > :grails-gsp-core:test
| Test | Runs | Flakiness |
|---|---|--:|
| GspCompileStaticConfigSpec > the report for a spaceship comes before the failure it does not prevent | β | [1% π‘](https://github.com/apache/grails-core/issues/16030) |
π·οΈ Commit: b715c0390b97b7f48ce210de5a4cc4d1d7689657
βΆοΈ Tests: 47081 executed
π‘ Checks: 41/90 completed
### Test Failures
<details><summary><strong>PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context</strong> (:grails-openapi:test in <a href="https://github.com/apache/grails-core/actions/runs/41558393873/job/125663790601?pr=16763">Groovy Snapshot Canary Build / Build Grails (shard 1)</a>)</summary>
```
Condition not satisfied:
!openApi.components.schemas['Widget'].properties.id?.readOnly
|| | | | | | |
|| | | | | | true
|| | | | | class IntegerSchema {
|| | | | | class Schema {
|| | | | | type: integer
|| | | | | format: int64
|| | | | | $ref: null
|| | | | | description: null
|| | | | | title: null
|| | | | | multipleOf: null
|| | | | | maximum: null
|| | | | | exclusiveMaximum: null
|| | | | | minimum: null
|| |β¦
```
</details>
<details><summary><strong>GspCompileStaticConfigSpec > the report for a spaceship comes before the failure it does not prevent</strong> (:grails-gsp-core:test in <a href="https://github.com/apache/grails-core/actions/runs/41558393873/job/125663790395?pr=16763">Groovy Snapshot Canary Build / Build Grails (shard 2)</a>)</summary>
```
Condition not satisfied:
template.metaInfo.compilationException.message.contains('General error during canonicalization')
| | | | |
| | | | false
| | | Could not parse script [Byte array resource [template-1859544565]]: startup failed:
| | | template_1859544565: 29: [Static type checking] - The type of [n] is not known here, and an operator cannot be applied to it. Declare it in the model directive, or give it a type where it is introduced.
| | | @ line 29, column 21.
| | | expressionOut.print(n <=> 2)
| | | ^
| | |
| | | 1 error
| | org.grails.gsp.GroovyPagesException: Could not parse script [Byte array resource [template-1859544565]]: stβ¦
```
</details>
<details><summary><strong>PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context</strong> (:grails-openapi:test in <a href="https://github.com/apache/grails-core/actions/runs/41558393825/job/125662826977?pr=16763">CI / Build Grails-Core (Windows JDK 25 shard 1)</a>)</summary>
```
Condition not satisfied:
!openApi.components.schemas['Widget'].properties.id?.readOnly
|| | | | | | |
|| | | | | | true
|| | | | | class IntegerSchema {
|| | | | | class Schema {
|| | | | | type: integer
|| | | | | format: int64
|| | | | | $ref: null
|| | | | | description: null
|| | | | | title: null
|| | | | | multipleOf: null
|| | | | | maximum: null
|| | | | | exclusiveMaximum: null
|| | | | | minimum: null
|| |β¦
```
</details>
<details><summary><strong>PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context</strong> (:grails-openapi:test in <a href="https://github.com/apache/grails-core/actions/runs/41558393825/job/125662827029?pr=16763">CI / Build Grails-Core Rerunning all Tasks (Ubuntu JDK 21 shard 1)</a>)</summary>
```
Condition not satisfied:
!openApi.components.schemas['Widget'].properties.id?.readOnly
|| | | | | | |
|| | | | | | true
|| | | | | class IntegerSchema {
|| | | | | class Schema {
|| | | | | type: integer
|| | | | | format: int64
|| | | | | $ref: null
|| | | | | description: null
|| | | | | title: null
|| | | | | multipleOf: null
|| | | | | maximum: null
|| | | | | exclusiveMaximum: null
|| | | | | minimum: null
|| |β¦
```
</details>
### Rerun Controls
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
π² GspCompileStaticConfigSpec > the report for a spaceship comes before the failure it does not prevent <!-- uniqueId=[engine:spock]/[spec:org.grails.gsp.GspCompileStaticConfigSpec]/[feature:$spock_feature_0_27] -->
π² PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context <!-- uniqueId=[engine:spock]/[spec:grails.openapi.PersistentEntitySchemaSpec]/[feature:$spock_feature_0_17] -->
Reuse successful test results:
π² β»οΈ Only rerun the tests that failed or were muted before
Click the checkbox to trigger a rerun:
π² **Rerun jobs**
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6432037517
Updated in commit a8501b95b8:
1. Moved the outer session physical connection acquisition before reading `previousConnectionHolder` and `getSynchronizations()` in `GrailsHibernateTemplate.executeWithNewSession`, ensuring any connection holder bound by the proxy is captured and cleanly restored in `finally`.
2. Changed `con.getMetaData()` to `con.getAutoCommit()`, documented the reason so it is preserved, and raised exception logging to `WARN`. This binds `TransactionAwareDataSourceProxy` without eagerly checking out a connection under `LazyConnectionDataSourceProxy`.
3. Removed unreachable `GenericJDBCException` check in `convertHibernateAccessException` and redundant `new HibernateSystemException` fallback in `doExecute`. Removed unused imports.
4. Added test coverage in `WithNewSessionLazyLoadSpec` covering `PROPAGATION_SUPPORTS, readOnly: true` with a disconnected request session (both with and without lazy loading inside the inner transaction), with table cleanup between tests.
5. Added test coverage in `GrailsHibernateTemplateSpec` verifying that an unmapped `SQLException` thrown as `GenericJDBCException` inside `template.execute` translates to `HibernateJdbcException`.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4418583534
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +232,60 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // This workaround is only taken when the connection provider uses TransactionAwareDataSourceProxy
+ // backed by LazyConnectionDataSourceProxy: con.getAutoCommit() allows LazyConnectionDataSourceProxy
+ // to bind the proxy without eagerly checking out a connection from the pool. When lazy wrapping is off
+ // or the data source is non-transaction-aware, acquiring the connection eagerly would check out a pooled
+ // connection unnecessarily and cause a single-connection pool to time out.
+ if (this.transactionAware && this.lazyConnection &&
+ previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getAutoCommit();
+ } catch (SQLException e) {
+ LOG.warn("Could not initialize physical connection: {}", e.getMessage(), e);
+ }
+ if (txResources.isSynchronizationActive() && !txResources.isActualTransactionActive()) {
+ // When nothing in the scope uses the outer session in a non-actual-transaction scope
+ // (such as PROPAGATION_SUPPORTS with synchronization), no session synchronization
+ // is registered, and the JDBC ConnectionSynchronization would close the connection
+ // the outer session still holds. Registering at SESSION_SYNCHRONIZATION_ORDER ensures
+ // the session is disconnected before Spring's ConnectionSynchronization runs.
+ // We strictly restrict this to scopes that are NOT an actual transaction to prevent
+ // manualDisconnect() from severing active write-bearing transactions before doCommit.
+ txResources.registerSynchronization(new TransactionSynchronization() {
+ @Override
+ public int getOrder() {
+ return SessionFactoryUtils.SESSION_SYNCHRONIZATION_ORDER;
+ }
+
+ @Override
+ public void beforeCompletion() {
Review Comment:
Addressed in commit 1b4eaafcd4:
1. Encapsulated outer session lifecycle management into explicit OuterSessionLifecycleStrategy implementations (ActiveTransactionOuterSessionStrategy, NonTransactionalSynchronizationOuterSessionStrategy, DefaultOuterSessionStrategy, and NoOpOuterSessionStrategy).
2. Implemented suspend() in NonTransactionalSynchronizationOuterSessionStrategy to call logicalConnection.manualDisconnect(). When an inner REQUIRED transaction begins and Spring suspends outer synchronizations, the primed session drops its connection immediately, preventing the inner transaction from reusing or closing it.
3. Implemented beforeCompletion() to disconnect before Spring's ConnectionSynchronization closes the physical connection when no inner transaction suspends the scope.
4. Added dedicated unit tests in OuterSessionLifecycleStrategySpec covering each strategy and interaction invariants in isolation, and added integration coverage in WithNewSessionLazyLoadSpec for a SUPPORTS scope executing a nested withNewSession followed by a REQUIRED transaction.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub) via notifications" <no...@grails.apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6539572784
## π¨ TestLens detected 2 failed tests π¨
### Failed Jobs without Test Failures
β [Groovy Snapshot Canary Build / Build Grails \(shard 1\)](https://github.com/apache/grails-core/actions/runs/41171210619/job/124449970801?pr=16763)
### Test Summary
> [!TIP]
> Inspect all details with the [TestLens CLI](https://testlens.app/docs/features/cli): `testlens pr 16763`
#### [CI / Build Grails-Core \(Windows JDK 25 shard 1\)](https://github.com/apache/grails-core/actions/runs/41171210330/job/124449381076?pr=16763) > :grails-openapi:test
| Test | Runs | Flakiness |
|---|---|--:|
| PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context | β | 0% π’ |
#### [CI / Build Grails-Core Rerunning all Tasks \(Ubuntu JDK 21 shard 1\)](https://github.com/apache/grails-core/actions/runs/41171210330/job/124449381153?pr=16763) > :grails-openapi:test
| Test | Runs | Flakiness |
|---|---|--:|
| PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context | β | 0% π’ |
π·οΈ Commit: bb56460f518248a1b22563e3e76c5589631da541
βΆοΈ Tests: 12113 executed
π‘ Checks: 41/90 completed
### Test Failures
<details><summary><strong>PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context</strong> (:grails-openapi:test in <a href="https://github.com/apache/grails-core/actions/runs/41171210330/job/124449381076?pr=16763">CI / Build Grails-Core (Windows JDK 25 shard 1)</a>)</summary>
```
Condition not satisfied:
!openApi.components.schemas['Widget'].properties.id?.readOnly
|| | | | | | |
|| | | | | | true
|| | | | | class IntegerSchema {
|| | | | | class Schema {
|| | | | | type: integer
|| | | | | format: int64
|| | | | | $ref: null
|| | | | | description: null
|| | | | | title: null
|| | | | | multipleOf: null
|| | | | | maximum: null
|| | | | | exclusiveMaximum: null
|| | | | | minimum: null
|| |β¦
```
</details>
<details><summary><strong>PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context</strong> (:grails-openapi:test in <a href="https://github.com/apache/grails-core/actions/runs/41171210330/job/124449381153?pr=16763">CI / Build Grails-Core Rerunning all Tasks (Ubuntu JDK 21 shard 1)</a>)</summary>
```
Condition not satisfied:
!openApi.components.schemas['Widget'].properties.id?.readOnly
|| | | | | | |
|| | | | | | true
|| | | | | class IntegerSchema {
|| | | | | class Schema {
|| | | | | type: integer
|| | | | | format: int64
|| | | | | $ref: null
|| | | | | description: null
|| | | | | title: null
|| | | | | multipleOf: null
|| | | | | maximum: null
|| | | | | exclusiveMaximum: null
|| | | | | minimum: null
|| |β¦
```
</details>
### Rerun Controls
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
π² PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context <!-- uniqueId=[engine:spock]/[spec:grails.openapi.PersistentEntitySchemaSpec]/[feature:$spock_feature_0_17] -->
Reuse successful test results:
π² β»οΈ Only rerun the tests that failed or were muted before
Click the checkbox to trigger a rerun:
π² **Rerun jobs**
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub) via notifications" <no...@grails.apache.org>.
borinquenkid commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4437058361
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -236,6 +239,30 @@ public <T> T executeWithNewSession(final Closure<T> callable) {
txResources.initSynchronization();
}
+ // If the outer session is disconnected, acquire its physical connection now before
+ // unbinding and entering the new session scope. Otherwise, lazy loading on outer session
+ // entities inside withNewSession { withTransaction { ... } } would borrow the inner
+ // transaction's connection and be left with a closed connection when the inner transaction commits.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getMetaData();
Review Comment:
Done in b986f36924. The comment on the priming call now says that `getPhysicalConnection()` returns the `TransactionAwareDataSourceProxy` handle, that `getAutoCommit()` is the first real call that makes the proxy bind its holder, and that it must not be removed as dead code. The lazy proxy keeps an idle session from checking out a pooled connection, and priming only runs for a lazy source.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -410,7 +437,8 @@ protected <T> T doExecute(HibernateCallback<T> action, boolean enforceNativeSess
flushIfNecessary(session, existingTransaction);
return result;
} catch (HibernateException ex) {
- throw convertHibernateAccessException(ex);
+ DataAccessException dae = convertHibernateAccessException(ex);
+ throw (dae != null ? dae : new HibernateSystemException(ex));
Review Comment:
At bb56460f51 `convertHibernateAccessException` has one fallback, in the `JDBCException` branch, for when the Spring translator returns null; the second fallback in `doExecute` is gone. `GrailsHibernateTemplateSpec` covers translation through `doExecute`. Please tell me if you still want the direct `convertHibernateAccessException` test from your snippet.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -680,10 +708,12 @@ protected void flushIfNecessary(Session session, boolean existingTransaction) th
@SuppressWarnings("ConstantConditions")
protected DataAccessException convertHibernateAccessException(HibernateException ex) {
if (ex instanceof JDBCException) {
- return convertJdbcAccessException((JDBCException) ex, jdbcExceptionTranslator);
+ DataAccessException dae = convertJdbcAccessException((JDBCException) ex, jdbcExceptionTranslator);
+ return dae != null ? dae : SessionFactoryUtils.convertHibernateAccessException(ex);
}
if (GenericJDBCException.class.equals(ex.getClass())) {
Review Comment:
Removed. There is no `GenericJDBCException` branch in `GrailsHibernateTemplate` at bb56460f51; the `instanceof JDBCException` check handles it.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +221,51 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // con.getAutoCommit() is used because TransactionAwareDataSourceProxy only obtains a target
+ // connection on its first real method call, binding the proxy to a target connection.
+ // Unlike getMetaData(), getAutoCommit() allows LazyConnectionDataSourceProxy to answer
+ // without eagerly checking out a connection from the pool when nothing is lazy-loaded.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
+ if (con != null) {
+ try {
+ con.getAutoCommit();
+ } catch (SQLException e) {
+ LOG.warn("Could not initialize physical connection: {}", e.getMessage(), e);
+ }
+ if (txResources.isSynchronizationActive()) {
Review Comment:
Done in b986f36924. The registration in `NonTransactionalSynchronizationOuterSessionStrategy.prepare()` now carries your one-line reason: when nothing in the scope uses the outer session, no session synchronization is registered, and Spring's JDBC `ConnectionSynchronization` would close the connection the primed session still holds.
##########
grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/WithNewSessionLazyLoadSpec.groovy:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.gorm.tests
+
+import grails.gorm.annotation.Entity
+import org.apache.grails.data.hibernate7.core.GrailsDataHibernate7TckManager
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import org.grails.orm.hibernate.proxy.HibernateProxyHandler
+import org.grails.orm.hibernate.support.hibernate7.SessionHolder
+import org.springframework.transaction.TransactionDefinition
+import org.springframework.transaction.support.TransactionSynchronizationManager
+import spock.lang.Issue
+
+/**
+ * Verifies that the request-bound session remains valid after a lazy load
+ * occurs within a nested withNewSession { withTransaction { ... } } block.
+ */
+@Issue('https://github.com/apache/grails-core/issues/16571')
+class WithNewSessionLazyLoadSpec extends GrailsDataTckSpec<GrailsDataHibernate7TckManager> {
+
+ void setupSpec() {
+ manager.isTransactional = false
+ manager.registerDomainClasses(WnsllBook, WnsllAuthor)
+ }
+
+ void cleanup() {
+ WnsllAuthor.withTransaction {
+ WnsllBook.where {}.deleteAll()
+ WnsllAuthor.where {}.deleteAll()
+ }
+ manager.sessionFactory.currentSession.clear()
+ }
+
+ void "request-bound session remains usable after lazy load inside withNewSession with inner transaction"() {
+ given: "a seeded author and book"
+ WnsllAuthor.withTransaction {
+ WnsllAuthor author = new WnsllAuthor(name: 'Stephen King').save()
+ new WnsllBook(title: 'The Stand', author: author).save()
+ }
+ manager.sessionFactory.currentSession.clear()
+
+ and: "an active request-bound session in TSM"
+ manager.sessionFactory.currentSession
+ SessionHolder requestSessionHolder = TransactionSynchronizationManager.getResource(manager.sessionFactory)
+
+ when: "loading an entity in the request session without a transaction"
+ WnsllBook book = WnsllBook.first()
+ HibernateProxyHandler proxyHandler = new HibernateProxyHandler()
+ assert !proxyHandler.isInitialized(book, 'author')
+
+ and: "running and ending a transaction on the request session"
+ WnsllAuthor.withTransaction { WnsllAuthor.count() }
+
+ and: "calling withNewSession with an inner transaction that lazy-loads an association on the request session entity"
+ WnsllBook.withNewSession {
+ WnsllBook.withTransaction {
+ assert book.author.name == 'Stephen King'
+ new WnsllAuthor(name: 'Tenar').save()
+ }
+ }
+
+ then: "the request-bound session remains open and usable"
+ requestSessionHolder.session.isOpen()
+ WnsllBook.count() == 1
+ WnsllAuthor.count() == 2
+ book.refresh()
+ book.title == 'The Stand'
+ book.author.name == 'Stephen King'
+ }
+
+ void "request-bound session remains usable after nested withNewSession inside SUPPORTS transaction"() {
+ given: "a seeded author and book"
+ WnsllAuthor.withTransaction {
+ WnsllAuthor author = new WnsllAuthor(name: 'Ursula').save()
+ new WnsllBook(title: 'Earthsea', author: author).save()
+ }
+ manager.sessionFactory.currentSession.clear()
+
+ and: "an active request-bound session in TSM"
+ manager.sessionFactory.currentSession
+ SessionHolder requestSessionHolder = TransactionSynchronizationManager.getResource(manager.sessionFactory)
+
+ when: "disconnecting the request session via an initial transaction"
+ WnsllAuthor.withTransaction { WnsllAuthor.count() }
+
+ and: "running a SUPPORTS read-only transaction with a nested withNewSession and inner transaction"
+ WnsllBook.withTransaction(propagationBehavior: TransactionDefinition.PROPAGATION_SUPPORTS, readOnly: true) {
+ WnsllBook.withNewSession {
+ WnsllBook.withTransaction { new WnsllAuthor(name: 'Tenar').save(flush: true) }
+ }
+ assert WnsllBook.count() == 1
+ }
+
+ then: "the request-bound session remains open and usable"
+ requestSessionHolder.session.isOpen()
+ WnsllBook.count() == 1
+ WnsllAuthor.count() == 2
+ }
+
+ void "request-bound session remains usable after nested withNewSession inside SUPPORTS transaction without in-scope query"() {
+ given: "a seeded author and book"
+ WnsllAuthor.withTransaction {
+ WnsllAuthor author = new WnsllAuthor(name: 'Ursula').save()
+ new WnsllBook(title: 'Earthsea', author: author).save()
+ }
+ manager.sessionFactory.currentSession.clear()
+
+ and: "an active request-bound session in TSM"
+ manager.sessionFactory.currentSession
+ SessionHolder requestSessionHolder = TransactionSynchronizationManager.getResource(manager.sessionFactory)
+
+ when: "disconnecting the request session via an initial transaction"
+ WnsllAuthor.withTransaction { WnsllAuthor.count() }
+
+ and: "running a SUPPORTS read-only transaction with a nested withNewSession without in-scope query"
+ WnsllBook.withTransaction(propagationBehavior: TransactionDefinition.PROPAGATION_SUPPORTS, readOnly: true) {
+ WnsllBook.withNewSession { WnsllAuthor.count() }
+ }
+
+ then: "the request-bound session remains open and usable after the scope completes"
+ requestSessionHolder.session.isOpen()
+ WnsllBook.count() == 1
+ }
+
+ void "request-bound session remains usable after lazy load inside withNewSession under SUPPORTS transaction"() {
+ given: "a seeded author and book"
+ WnsllAuthor.withTransaction {
+ WnsllAuthor author = new WnsllAuthor(name: 'Ursula').save()
+ new WnsllBook(title: 'Earthsea', author: author).save()
+ }
+ manager.sessionFactory.currentSession.clear()
+
+ and: "an active request-bound session in TSM"
+ manager.sessionFactory.currentSession
+ SessionHolder requestSessionHolder = TransactionSynchronizationManager.getResource(manager.sessionFactory)
+
+ when: "loading an entity in the request session without a transaction"
+ WnsllBook book = WnsllBook.first()
+ HibernateProxyHandler proxyHandler = new HibernateProxyHandler()
+ assert !proxyHandler.isInitialized(book, 'author')
+
+ and: "disconnecting the request session via an initial transaction"
+ WnsllAuthor.withTransaction { WnsllAuthor.count() }
+
+ and: "calling withNewSession under a SUPPORTS scope with an inner transaction that lazy-loads"
+ WnsllBook.withTransaction(propagationBehavior: TransactionDefinition.PROPAGATION_SUPPORTS, readOnly: true) {
+ WnsllBook.withNewSession {
+ WnsllBook.withTransaction {
+ assert book.author.name == 'Ursula'
+ new WnsllAuthor(name: 'Tenar').save(flush: true)
+ }
+ }
+ assert WnsllBook.count() == 1
Review Comment:
The `WithNewSessionLazyLoadSpec` feature "request-bound session remains usable after nested withNewSession inside SUPPORTS transaction without in-scope query" leaves the scope without touching the outer session and queries it afterwards, so it needs the synchronization. The older feature that queries inside the scope is still there as well.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -220,6 +232,60 @@ public <T> T execute(Closure<T> callable) {
public <T> T executeWithNewSession(final Closure<T> callable) {
SessionHolder sessionHolder = (SessionHolder) txResources.getResource(sessionFactory);
SessionHolder previousHolder = sessionHolder;
+
+ // If the outer session is open but physically disconnected, acquire its physical connection
+ // before saving previousConnectionHolder and synchronizations, and before unbinding and entering
+ // the new session scope. Otherwise, lazy loading on outer session entities inside
+ // withNewSession { withTransaction { ... } } would borrow the inner transaction's
+ // thread-bound connection and be left with a closed connection when the inner transaction commits.
+ // This workaround is only taken when the connection provider uses TransactionAwareDataSourceProxy
+ // backed by LazyConnectionDataSourceProxy: con.getAutoCommit() allows LazyConnectionDataSourceProxy
+ // to bind the proxy without eagerly checking out a connection from the pool. When lazy wrapping is off
+ // or the data source is non-transaction-aware, acquiring the connection eagerly would check out a pooled
+ // connection unnecessarily and cause a single-connection pool to time out.
+ if (this.transactionAware && this.lazyConnection &&
Review Comment:
Addressed in bb56460f51, with a different shape than the thread asked for. An idle outer session is not primed at all when `dataSource.lazy=false`. On restore, a session that was not connected on entry and is connected at the end is disconnected, so it never keeps the inner transaction's connection. `WithNewSessionNonLazyPoolSpec` runs against a one-connection pool with `lazy=false`, including "lazy loading in inner transaction with non-lazy pool drops closed connection on restore".
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub) via notifications" <no...@grails.apache.org>.
borinquenkid commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4437059252
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/AbstractOuterSessionLifecycleStrategy.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.orm.hibernate;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.List;
+
+import javax.sql.DataSource;
+
+import org.hibernate.Session;
+import org.hibernate.SessionFactory;
+import org.hibernate.engine.jdbc.spi.JdbcCoordinator;
+import org.hibernate.engine.spi.SessionImplementor;
+import org.hibernate.resource.jdbc.spi.LogicalConnectionImplementor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.springframework.jdbc.datasource.ConnectionHolder;
+import org.springframework.transaction.support.TransactionSynchronization;
+
+import org.grails.orm.hibernate.support.hibernate7.SessionHolder;
+import org.grails.orm.hibernate.support.hibernate7.TransactionResources;
+
+/**
+ * Base strategy handling symmetric capture, unbinding, and restoration of session and connection holders
+ * as well as Spring transaction synchronizations.
+ */
+abstract class AbstractOuterSessionLifecycleStrategy implements OuterSessionLifecycleStrategy {
+
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractOuterSessionLifecycleStrategy.class);
+
+ protected final SessionFactory sessionFactory;
+ protected final DataSource dataSource;
+ protected final TransactionResources txResources;
+ protected final SessionHolder previousHolder;
+
+ protected ConnectionHolder previousConnectionHolder;
+ protected boolean previousActiveSynchronization;
+ protected List<TransactionSynchronization> transactionSynchronizations;
+
+ AbstractOuterSessionLifecycleStrategy(
+ SessionFactory sessionFactory,
+ DataSource dataSource,
+ TransactionResources txResources,
+ SessionHolder previousHolder) {
+ this.sessionFactory = sessionFactory;
+ this.dataSource = dataSource;
+ this.txResources = txResources;
+ this.previousHolder = previousHolder;
+ }
+
+ protected void primeIfDisconnected() {
+ if (previousHolder == null || previousHolder.getSession() == null || !previousHolder.getSession().isOpen()) {
+ return;
+ }
+
+ Session previousSession = previousHolder.getSession();
+ if (!(previousSession instanceof SessionImplementor sessionImpl)) {
+ return;
+ }
+
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator == null) {
+ return;
+ }
+
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection == null || logicalConnection.isPhysicallyConnected()) {
+ return;
+ }
+
+ Connection con = logicalConnection.getPhysicalConnection();
Review Comment:
Priming is now gated on a lazy connection proxy (`lazyConnection`) in both `NonTransactionalSynchronizationOuterSessionStrategy.prepare()` and the restore path, so a non-lazy source, transaction-aware or not, never checks out its only pooled connection for an idle outer session. The gate is the lazy proxy rather than a transaction-aware check; if you want it keyed on the transaction-aware proxy instead, tell me which case it misses. The one-connection `lazy=false` scenarios are in `WithNewSessionNonLazyPoolSpec`.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/OuterSessionLifecycleStrategy.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.orm.hibernate;
+
+import javax.sql.DataSource;
+
+import org.hibernate.SessionFactory;
+
+import org.grails.orm.hibernate.support.hibernate7.SessionHolder;
+import org.grails.orm.hibernate.support.hibernate7.TransactionResources;
+
+/**
+ * Strategy interface governing outer session lifecycle preparation, suspension, and restoration
+ * across nested {@code withNewSession} invocations.
+ */
+interface OuterSessionLifecycleStrategy {
+
+ /**
+ * Prepares the outer session before unbinding and opening the new session.
+ */
+ void prepare();
+
+ /**
+ * Suspends any active Spring synchronizations and unbinds outer session and connection holders.
+ */
+ void suspendAndUnbind();
+
+ /**
+ * Restores previously suspended synchronizations and rebinds outer session and connection holders.
+ */
+ void restore();
+
+ /**
+ * Factory method creating the appropriate strategy based on the outer session, connection, and transaction context.
+ *
+ * @param previousHolder the session holder bound to the outer scope, or null
+ * @param sessionFactory the current Hibernate SessionFactory
+ * @param dataSource the underlying DataSource
+ * @param txResources the transaction synchronization and resource manager
+ * @return the appropriate {@link OuterSessionLifecycleStrategy}
+ */
+ static OuterSessionLifecycleStrategy from(
+ SessionHolder previousHolder,
+ SessionFactory sessionFactory,
+ DataSource dataSource,
+ TransactionResources txResources) {
+
+ boolean hasOpenSession = previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen();
+ boolean hasConnectionHolder = txResources.getResource(dataSource) != null;
+ boolean hasSynchronization = txResources.isSynchronizationActive();
+
+ if (!hasOpenSession && !hasConnectionHolder && !hasSynchronization) {
Review Comment:
Fixed in bb56460f51. `OuterSessionLifecycleStrategy.from` returns the no-op strategy only when no session holder is bound, no connection holder is bound and no synchronization is active. A bound holder whose session is closed goes to `DefaultOuterSessionStrategy`, which unbinds it and rebinds it afterwards. Covered by "withNewSession runs and rebinds holder when outer request session was closed before scope" in `WithNewSessionNonLazyPoolSpec`.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6432951904
## β
All tests passed β
π·οΈ Commit: a8501b95b897fcd77e3a53e425f45905c842c0bb
βΆοΈ Tests: 99412 executed
βͺοΈ Checks: 96/96 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6454387086
## β
All tests passed β
π·οΈ Commit: cba1b7f74ee4870329ea1592830dfc76e3703021
βΆοΈ Tests: 92107 executed
βͺοΈ Checks: 96/96 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6488621613
## β
All tests passed β
π·οΈ Commit: fe2490938fcc1259fe6c18c1a7ff43ea969a1553
βΆοΈ Tests: 92257 executed
βͺοΈ Checks: 96/96 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub) via notifications" <no...@grails.apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6563067080
## π¨ TestLens detected 1 failed test π¨
### Test Summary
> [!TIP]
> Inspect all details with the [TestLens CLI](https://testlens.app/docs/features/cli): `testlens pr 16763`
#### [CI / Build Grails-Core Rerunning all Tasks \(Ubuntu JDK 21 shard 1\)](https://github.com/apache/grails-core/actions/runs/41342776616/job/124956145150?pr=16763) > :grails-openapi:test
| Test | Runs | Flakiness |
|---|---|--:|
| PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context | β | 0% π’ |
π·οΈ Commit: b986f36924d9ed8effb328f75d60033e059c3837
βΆοΈ Tests: 36418 executed
π‘ Checks: 39/90 completed
### Test Failures
<details><summary><strong>PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context</strong> (:grails-openapi:test in <a href="https://github.com/apache/grails-core/actions/runs/41342776616/job/124956145150?pr=16763">CI / Build Grails-Core Rerunning all Tasks (Ubuntu JDK 21 shard 1)</a>)</summary>
```
Condition not satisfied:
!openApi.components.schemas['Widget'].properties.id?.readOnly
|| | | | | | |
|| | | | | | true
|| | | | | class IntegerSchema {
|| | | | | class Schema {
|| | | | | type: integer
|| | | | | format: int64
|| | | | | $ref: null
|| | | | | description: null
|| | | | | title: null
|| | | | | multipleOf: null
|| | | | | maximum: null
|| | | | | exclusiveMaximum: null
|| | | | | minimum: null
|| |β¦
```
</details>
### Rerun Controls
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
π² PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context <!-- uniqueId=[engine:spock]/[spec:grails.openapi.PersistentEntitySchemaSpec]/[feature:$spock_feature_0_17] -->
Reuse successful test results:
π² β»οΈ Only rerun the tests that failed or were muted before
Click the checkbox to trigger a rerun:
π² **Rerun jobs**
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "borinquenkid (via GitHub) via notifications" <no...@grails.apache.org>.
borinquenkid commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6537462904
Updated branch `fix/h7-withnewsession-connection-8.0.x` in commit `67880b478c` bringing full architectural parity with the Hibernate 5 implementation on #16784:
1. **Lazy Connection Priming & Restore Disconnect**:
- `NonTransactionalSynchronizationOuterSessionStrategy`: Gated priming during `prepare()` to `lazyConnection && !wasPhysicallyConnectedAtPrepare`.
- `AbstractOuterSessionLifecycleStrategy`: Disconnects on `restore()` if the session was not physically connected at `prepare()` and either the data source is non-lazy or no synchronization was active.
- `DefaultOuterSessionStrategy`: Removed eager priming so idle outer sessions never check out connections on non-lazy connection pools.
2. **Session Holder & Resource Safety**:
- Closed outer sessions preserve their bound `SessionHolder` across nested scopes by routing `previousHolder != null` to `DefaultOuterSessionStrategy`.
- All `dataSource` transaction resource lookups, unbinds, and restores are guarded against `null`.
3. **Exception Translation & Flush Delegation**:
- `GrailsHibernateTemplate.executeWithNewSession`: Delegates execution through `execute(callable::call)` so that flush mode rules and exception translation to Spring `DataAccessException` apply.
- Streamlined `cleanupNewSession` to unbind resources via `unbindResourceIfPossible` without broad catch blocks.
4. **Test Coverage**:
- Added `WithNewSessionNonLazyPoolSpec.groovy` to `grails-data-hibernate7-core` testing single-connection non-lazy pools (`maxActive=1`, `dataSource.lazy=false`) across outer transaction disconnect, inner transaction write, `PROPAGATION_SUPPORTS` scopes, and closed outer session restoration.
- Extended `GrailsHibernateTemplateSpec.groovy` to verify eager flush exception translation (`DataIntegrityViolationException`) and session holder restoration on `openSession()` failure.
- Updated `OuterSessionLifecycleStrategySpec.groovy` with all 8 strategy factory and lifecycle test cases.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#discussion_r4373361074
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -236,6 +239,30 @@ public <T> T executeWithNewSession(final Closure<T> callable) {
txResources.initSynchronization();
}
+ // If the outer session is disconnected, acquire its physical connection now before
+ // unbinding and entering the new session scope. Otherwise, lazy loading on outer session
+ // entities inside withNewSession { withTransaction { ... } } would borrow the inner
+ // transaction's connection and be left with a closed connection when the inner transaction commits.
+ if (previousHolder != null && previousHolder.getSession() != null && previousHolder.getSession().isOpen()) {
+ Session previousSession = previousHolder.getSession();
+ if (previousSession instanceof SessionImplementor sessionImpl) {
+ JdbcCoordinator jdbcCoordinator = sessionImpl.getJdbcCoordinator();
+ if (jdbcCoordinator != null) {
+ LogicalConnectionImplementor logicalConnection = jdbcCoordinator.getLogicalConnection();
+ if (logicalConnection != null && !logicalConnection.isPhysicallyConnected()) {
+ Connection con = logicalConnection.getPhysicalConnection();
Review Comment:
This acquires the outer connection after `previousConnectionHolder` was saved as null, and after synchronization was suspended. `TransactionAwareDataSourceProxy` binds a new holder. The cleanup does not unbind it, because it still thinks there was none. A nested `withTransaction` can then fail with "Already value bound", and cleanup can release the outer connection.
Acquire that connection before saving the holder and the synchronization list. Please cover a `SUPPORTS` scope with a disconnected request session and no bound DataSource holder.
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hibernate 7: prevent outer session connection closure during nested withNewSession lazy loading [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16763:
URL: https://github.com/apache/grails-core/pull/16763#issuecomment-6406548271
## β
All tests passed β
π·οΈ Commit: 4695e2a1c6407cae68547e47bbb01954419647d7
βΆοΈ Tests: 99409 executed
βͺοΈ Checks: 98/98 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org