jamesfredley commented on code in PR #15425:
URL: https://github.com/apache/grails-core/pull/15425#discussion_r2835768249
##########
grails-data-hibernate5/grails-plugin/src/main/groovy/org/grails/plugin/hibernate/support/GrailsOpenSessionInViewInterceptor.java:
##########
@@ -73,6 +120,46 @@ public void postHandle(WebRequest request, ModelMap model)
throws DataAccessExce
session.setHibernateFlushMode(FlushMode.MANUAL);
}
}
+
+ for (AdditionalSessionFactoryConfig config :
additionalSessionFactories) {
+ SessionFactory sf = config.sessionFactory;
+ SessionHolder additionalHolder = (SessionHolder)
TransactionSynchronizationManager.getResource(sf);
+ if (additionalHolder != null) {
+ Session additionalSession = additionalHolder.getSession();
+ try {
+ FlushMode additionalFlushMode =
additionalSession.getHibernateFlushMode();
+ boolean additionalIsNotManual = additionalFlushMode !=
FlushMode.MANUAL && additionalFlushMode != FlushMode.COMMIT;
+ if (additionalIsNotManual) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Eagerly flushing additional
Hibernate session");
+ }
+ additionalSession.flush();
+ }
+ }
+ finally {
+ additionalSession.setHibernateFlushMode(FlushMode.MANUAL);
+ }
+ }
+ }
+ }
+
+ @Override
+ public void afterCompletion(WebRequest request, Exception ex) throws
DataAccessException {
+ for (int i = additionalSessionFactories.size() - 1; i >= 0; i--) {
+ AdditionalSessionFactoryConfig config =
additionalSessionFactories.get(i);
+ SessionFactory sf = config.sessionFactory;
+ SessionHolder sessionHolder = (SessionHolder)
TransactionSynchronizationManager.getResource(sf);
+ if (sessionHolder != null) {
+ Session session = sessionHolder.getSession();
+ TransactionSynchronizationManager.unbindResource(sf);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Closing additional Hibernate Session in
OpenSessionInViewInterceptor");
+ }
+ SessionFactoryUtils.closeSession(session);
+ }
+ }
+
+ super.afterCompletion(request, ex);
Review Comment:
Valid point - addressed in 90f9ff78ca. Wrapped the cleanup loop in
try-finally to guarantee `super.afterCompletion()` runs (protecting the default
session), and added per-session try-catch around `closeSession` with error
logging. This matches the pattern used by
`HibernatePersistenceContextInterceptor.destroy()` in this same module.
##########
grails-data-hibernate5/grails-plugin/src/main/groovy/org/grails/plugin/hibernate/support/GrailsOpenSessionInViewInterceptor.java:
##########
@@ -53,6 +81,25 @@ protected void applyFlushMode(Session session) {
session.setHibernateFlushMode(hibernateFlushMode);
}
+ @Override
+ public void preHandle(WebRequest request) throws DataAccessException {
+ super.preHandle(request);
+
+ for (AdditionalSessionFactoryConfig config :
additionalSessionFactories) {
+ SessionFactory sf = config.sessionFactory;
+ if (TransactionSynchronizationManager.hasResource(sf)) {
+ continue;
+ }
+ if (logger.isDebugEnabled()) {
+ logger.debug("Opening additional Hibernate Session for
datasource in OpenSessionInViewInterceptor");
+ }
+ Session session = sf.openSession();
+ session.setHibernateFlushMode(config.flushMode);
+ SessionHolder sessionHolder = new SessionHolder(session);
+ TransactionSynchronizationManager.bindResource(sf, sessionHolder);
Review Comment:
Not adopting this one. Spring MVC's `DispatcherServlet` calls
`triggerAfterCompletion` when `preHandle` throws, so sessions bound before the
failure are cleaned up through the normal `afterCompletion` path. Spring's own
`OpenSessionInViewInterceptor.preHandle()` has the same exposure (if
`openSession()` or `bindResource()` throws, the session leaks) and makes no
attempt to defend against it. The suggested rollback logic (~25 lines)
duplicates what `afterCompletion` already handles and adds complexity for an
extremely unlikely scenario (SessionFactory broken mid-request).
##########
grails-data-hibernate5/grails-plugin/src/main/groovy/org/grails/plugin/hibernate/support/GrailsOpenSessionInViewInterceptor.java:
##########
@@ -84,5 +171,25 @@ public void
setHibernateDatastore(AbstractHibernateDatastore hibernateDatastore)
this.hibernateFlushMode = FlushMode.valueOf(defaultFlushModeName);
}
setSessionFactory(hibernateDatastore.getSessionFactory());
+
+ if (hibernateDatastore instanceof HibernateDatastore) {
+ HibernateDatastore hibernateDs = (HibernateDatastore)
hibernateDatastore;
+ for (ConnectionSource<SessionFactory,
HibernateConnectionSourceSettings> connectionSource :
hibernateDs.getConnectionSources().getAllConnectionSources()) {
+ String connectionName = connectionSource.getName();
+ if (!ConnectionSource.DEFAULT.equals(connectionName)) {
+ AbstractHibernateDatastore childDatastore =
hibernateDs.getDatastoreForConnection(connectionName);
+ FlushMode childFlushMode;
+ if (childDatastore.isOsivReadOnly()) {
+ childFlushMode = FlushMode.MANUAL;
+ }
+ else {
+ childFlushMode =
FlushMode.valueOf(childDatastore.getDefaultFlushModeName());
+ }
+ additionalSessionFactories.add(
+ new
AdditionalSessionFactoryConfig(childDatastore.getSessionFactory(),
childFlushMode)
+ );
+ }
+ }
+ }
Review Comment:
Not applicable - `setHibernateDatastore` is a Spring property setter invoked
once during bean initialization via `hibernateDatastore =
ref('hibernateDatastore')` in `HibernateDatastoreSpringInitializer`. There is
no code path that calls it multiple times on the same interceptor instance.
##########
grails-data-hibernate5/grails-plugin/src/main/groovy/org/grails/plugin/hibernate/support/GrailsOpenSessionInViewInterceptor.java:
##########
@@ -73,6 +120,46 @@ public void postHandle(WebRequest request, ModelMap model)
throws DataAccessExce
session.setHibernateFlushMode(FlushMode.MANUAL);
}
}
+
+ for (AdditionalSessionFactoryConfig config :
additionalSessionFactories) {
+ SessionFactory sf = config.sessionFactory;
+ SessionHolder additionalHolder = (SessionHolder)
TransactionSynchronizationManager.getResource(sf);
+ if (additionalHolder != null) {
+ Session additionalSession = additionalHolder.getSession();
+ try {
+ FlushMode additionalFlushMode =
additionalSession.getHibernateFlushMode();
+ boolean additionalIsNotManual = additionalFlushMode !=
FlushMode.MANUAL && additionalFlushMode != FlushMode.COMMIT;
+ if (additionalIsNotManual) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Eagerly flushing additional
Hibernate session");
+ }
+ additionalSession.flush();
+ }
+ }
+ finally {
+ additionalSession.setHibernateFlushMode(FlushMode.MANUAL);
+ }
+ }
+ }
Review Comment:
Not adopting this one. The additional session flush follows the same pattern
as the default session flush (lines 107-122) - neither accumulates exceptions.
If `session.flush()` throws, it signals a data integrity problem (constraint
violation, stale data) where fail-fast is the correct behavior.
`afterCompletion` still runs after `postHandle` throws (Spring MVC contract),
so all sessions are properly cleaned up regardless.
--
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]
