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