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