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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
