jdaugherty commented on code in PR #16784:
URL: https://github.com/apache/grails-core/pull/16784#discussion_r4419406821
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -85,6 +86,9 @@ public class GrailsHibernateTemplate implements
IHibernateTemplate {
protected SQLExceptionTranslator jdbcExceptionTranslator;
protected int flushMode = FLUSH_AUTO;
private boolean applyFlushModeOnlyToNonExistingTransactions = false;
+ private boolean transactionAware = false;
+ private boolean lazyConnection = false;
Review Comment:
Both flags are computed and never read; the lazy gate on the priming went
with them, which is the `dataSource.lazy=false` regression in the summary (item
1). The gate belongs here: prime only when `transactionAware && lazyConnection`.
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/DefaultOuterSessionStrategy.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.springframework.orm.hibernate5.SessionHolder;
+
+import org.grails.orm.hibernate.support.TransactionResources;
+
+/**
+ * Strategy applied when an outer session exists but no Spring transaction
synchronization is active.
+ *
+ * <p>Primes a disconnected outer session so that {@code
txResources.getResource(dataSource)} captures
+ * its {@code ConnectionHolder} before the new session begins, preventing
outer session lazy loading
+ * inside {@code withNewSession} from borrowing the inner transaction's
connection.</p>
+ */
+class DefaultOuterSessionStrategy extends
AbstractOuterSessionLifecycleStrategy {
+
+ DefaultOuterSessionStrategy(
+ SessionFactory sessionFactory,
+ DataSource dataSource,
+ TransactionResources txResources,
+ SessionHolder previousHolder) {
+ super(sessionFactory, dataSource, txResources, previousHolder);
+ }
+
+ @Override
+ public void prepare() {
+ primeIfDisconnected();
Review Comment:
This strategy runs for an OSIV request outside any transaction scope. It
primes but registers nothing that disconnects, so with `dataSource.lazy=false`
the request session keeps the pooled connection it checked out here until the
session closes (one active connection after `Book.withNewSession {
Author.count() }`; 7.0.x has zero), and with the default lazy proxy it keeps
whatever connection a lazy load fetched during the scope. Disconnecting in
`restore()` when the session was not physically connected at `prepare()` covers
both.
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -149,89 +165,65 @@ public <T> T execute(Closure<T> callable) {
return execute(hibernateCallback);
}
+ @SuppressWarnings("PMD.DataflowAnomalyAnalysis")
@Override
public <T> T executeWithNewSession(final Closure<T> callable) {
- SessionHolder sessionHolder = (SessionHolder)
TransactionSynchronizationManager.getResource(sessionFactory);
- SessionHolder previousHolder = sessionHolder;
- ConnectionHolder previousConnectionHolder = (ConnectionHolder)
TransactionSynchronizationManager.getResource(dataSource);
+ SessionHolder sessionHolder = (SessionHolder)
txResources.getResource(sessionFactory);
+ OuterSessionLifecycleStrategy strategy =
OuterSessionLifecycleStrategy.from(
+ sessionHolder, sessionFactory, dataSource, txResources);
+
+ strategy.prepare();
Session newSession = null;
- boolean previousActiveSynchronization =
TransactionSynchronizationManager.isSynchronizationActive();
- List<TransactionSynchronization> transactionSynchronizations =
previousActiveSynchronization ?
TransactionSynchronizationManager.getSynchronizations() : null;
try {
- // if there are any previous synchronizations active we need to
clear them and restore them later (see finally block)
- if (previousActiveSynchronization) {
- TransactionSynchronizationManager.clearSynchronization();
- // init a new synchronization to ensure that any opened
database connections are closed by the synchronization
- TransactionSynchronizationManager.initSynchronization();
- }
-
- // if there are already bound holders, unbind them so they can be
restored later
- if (sessionHolder != null) {
-
TransactionSynchronizationManager.unbindResource(sessionFactory);
- if (previousConnectionHolder != null) {
-
TransactionSynchronizationManager.unbindResource(dataSource);
- }
- }
+ strategy.suspendAndUnbind();
- // create and bind a new session holder for the new session
newSession = sessionFactory.openSession();
applyFlushMode(newSession, false);
sessionHolder = new SessionHolder(newSession);
- TransactionSynchronizationManager.bindResource(sessionFactory,
sessionHolder);
+ txResources.bindResource(sessionFactory, sessionHolder);
- return execute(callable::call);
+ return callable.call(newSession);
}
finally {
- try {
- // if an active synchronization was registered during the life
time of the new session clear it
- if
(TransactionSynchronizationManager.isSynchronizationActive()) {
- TransactionSynchronizationManager.clearSynchronization();
- }
- // If there is a synchronization active then leave it to the
synchronization to close the session
- if (newSession != null) {
- SessionFactoryUtils.closeSession(newSession);
- }
+ cleanupNewSession(newSession);
+ strategy.restore();
+ }
+ }
- // Clear any bound sessions and connections
-
TransactionSynchronizationManager.unbindResource(sessionFactory);
- ConnectionHolder connectionHolder = (ConnectionHolder)
TransactionSynchronizationManager.unbindResourceIfPossible(dataSource);
- // if there is a connection holder and it holds an open
connection close it
- try {
- if (connectionHolder != null &&
!connectionHolder.getConnection().isClosed()) {
- Connection conn = connectionHolder.getConnection();
- DataSourceUtils.releaseConnection(conn, dataSource);
- }
- } catch (SQLException e) {
- // ignore, connection closed already?
- if (LOG.isDebugEnabled()) {
- LOG.debug("Could not close opened JDBC connection. Did
the application close the connection manually?: " + e.getMessage());
- }
- }
+ private void cleanupNewSession(Session newSession) {
+ try {
+ if (txResources.isSynchronizationActive()) {
+ txResources.clearSynchronization();
}
- finally {
- // if there were previously active synchronizations then
register those again
- if (previousActiveSynchronization) {
- TransactionSynchronizationManager.initSynchronization();
- for (TransactionSynchronization transactionSynchronization
: transactionSynchronizations) {
-
TransactionSynchronizationManager.registerSynchronization(transactionSynchronization);
- }
+ txResources.unbindResource(sessionFactory);
+ ConnectionHolder connectionHolder =
+ (ConnectionHolder)
txResources.unbindResourceIfPossible(dataSource);
+ try {
+ if (connectionHolder != null &&
+ !(dataSource instanceof
org.grails.datastore.gorm.jdbc.MultiTenantDataSource) &&
+ !connectionHolder.getConnection().isClosed()) {
+ Connection conn = connectionHolder.getConnection();
+ DataSourceUtils.releaseConnection(conn, dataSource);
}
-
- // now restore any previous state
- if (previousHolder != null) {
-
TransactionSynchronizationManager.bindResource(sessionFactory, previousHolder);
- if (previousConnectionHolder != null) {
-
TransactionSynchronizationManager.bindResource(dataSource,
previousConnectionHolder);
- }
+ } catch (SQLException e) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Could not close opened JDBC connection. Did the
application close the connection manually?: " +
+ e.getMessage());
}
+ }
+ if (newSession != null) {
+ SessionFactoryUtils.closeSession(newSession);
}
+ } catch (RuntimeException e) {
Review Comment:
Non-blocking: this also swallows a failure of
`SessionFactoryUtils.closeSession(newSession)`, so a session that could not be
closed is only a WARN line. If the catch is here for the unbind throwing when
`openSession()` failed and nothing was bound,
`unbindResourceIfPossible(sessionFactory)` covers that without hiding a close
failure.
##########
grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/OuterSessionLifecycleStrategySpec.groovy:
##########
@@ -0,0 +1,416 @@
+/*
+ * 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 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.springframework.jdbc.datasource.ConnectionHolder
+import org.springframework.orm.hibernate5.SessionFactoryUtils
+import org.springframework.orm.hibernate5.SessionHolder
+import org.springframework.transaction.support.TransactionSynchronization
+import spock.lang.Specification
+
+import org.grails.orm.hibernate.support.TransactionResources
+
+class OuterSessionLifecycleStrategySpec extends Specification {
Review Comment:
These features build the package-private strategies directly with a mocked
`TransactionResources` and assert the calls the code makes; none of them fails
for item 1. The branches that matter are reachable through `withNewSession`
with a `HibernateDatastore` built from a config map, as
`GrailsHibernateTemplateSpec` does: `dataSource.lazy=false`, a pool with
`maxActive=1`, and a bound session with no synchronization active. Could you
cover those there instead of this spec?
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/tests/WithNewSessionLazyLoadSpec.groovy:
##########
@@ -0,0 +1,258 @@
+/*
+ * 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.hibernate5.core.GrailsDataHibernate5TckManager
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import org.grails.orm.hibernate.proxy.HibernateProxyHandler
+import org.springframework.orm.hibernate5.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/16783')
+class WithNewSessionLazyLoadSpec extends
GrailsDataTckSpec<GrailsDataHibernate5TckManager> {
+
+ void setupSpec() {
+ manager.isTransactional = false
+ manager.domainClasses.addAll([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)
+ }
+ }
+ }
+
+ then: "the request-bound session remains open and usable"
+ requestSessionHolder.session.isOpen()
+ WnsllBook.count() == 1
+ WnsllAuthor.count() == 2
+ book.refresh()
+ book.title == 'Earthsea'
+ book.author.name == 'Ursula'
+ }
+
+ void "request-bound session remains usable after nested withNewSession
inside SUPPORTS transaction without in-scope query"() {
Review Comment:
Same feature name as line 116; Spock runs both and the report lists the name
twice. This one runs without the seeded rows; if that is the point, name it so,
otherwise drop one of them.
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -149,89 +165,65 @@ public <T> T execute(Closure<T> callable) {
return execute(hibernateCallback);
}
+ @SuppressWarnings("PMD.DataflowAnomalyAnalysis")
@Override
public <T> T executeWithNewSession(final Closure<T> callable) {
- SessionHolder sessionHolder = (SessionHolder)
TransactionSynchronizationManager.getResource(sessionFactory);
- SessionHolder previousHolder = sessionHolder;
- ConnectionHolder previousConnectionHolder = (ConnectionHolder)
TransactionSynchronizationManager.getResource(dataSource);
+ SessionHolder sessionHolder = (SessionHolder)
txResources.getResource(sessionFactory);
+ OuterSessionLifecycleStrategy strategy =
OuterSessionLifecycleStrategy.from(
+ sessionHolder, sessionFactory, dataSource, txResources);
+
+ strategy.prepare();
Session newSession = null;
- boolean previousActiveSynchronization =
TransactionSynchronizationManager.isSynchronizationActive();
- List<TransactionSynchronization> transactionSynchronizations =
previousActiveSynchronization ?
TransactionSynchronizationManager.getSynchronizations() : null;
try {
- // if there are any previous synchronizations active we need to
clear them and restore them later (see finally block)
- if (previousActiveSynchronization) {
- TransactionSynchronizationManager.clearSynchronization();
- // init a new synchronization to ensure that any opened
database connections are closed by the synchronization
- TransactionSynchronizationManager.initSynchronization();
- }
-
- // if there are already bound holders, unbind them so they can be
restored later
- if (sessionHolder != null) {
-
TransactionSynchronizationManager.unbindResource(sessionFactory);
- if (previousConnectionHolder != null) {
-
TransactionSynchronizationManager.unbindResource(dataSource);
- }
- }
+ strategy.suspendAndUnbind();
- // create and bind a new session holder for the new session
newSession = sessionFactory.openSession();
applyFlushMode(newSession, false);
sessionHolder = new SessionHolder(newSession);
- TransactionSynchronizationManager.bindResource(sessionFactory,
sessionHolder);
+ txResources.bindResource(sessionFactory, sessionHolder);
- return execute(callable::call);
+ return callable.call(newSession);
Review Comment:
Item 2 in the summary: `Book.withNewSession { Session s -> s.get(Book,
'not-a-long') }` throws `HibernateSystemException` on 7.0.x and
`org.hibernate.TypeMismatchException` here. Please keep
`execute(callable::call)`.
--
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]
