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