Copilot commented on code in PR #15425:
URL: https://github.com/apache/grails-core/pull/15425#discussion_r2834499370
##########
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:
The cleanup logic in the loop should be wrapped in a try-catch block to
ensure all sessions are closed even if closing one session throws an exception.
If an exception occurs while closing one of the additional sessions (e.g., line
158), subsequent sessions in the list won't be closed, and the default session
cleanup (line 162) won't be called. Consider wrapping the session cleanup in
each iteration with try-catch similar to how
HibernatePersistenceContextInterceptor handles it (line 99).
```suggestion
try {
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");
}
try {
SessionFactoryUtils.closeSession(session);
}
catch (RuntimeException closeEx) {
if (logger.isErrorEnabled()) {
logger.error("Unexpected exception on closing
additional Hibernate Session", closeEx);
}
}
}
}
}
finally {
super.afterCompletion(request, ex);
}
```
##########
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:
The additionalSessionFactories list is not cleared before being populated in
setHibernateDatastore. If this method is called multiple times (e.g., during
bean reconfiguration or in tests), the list will accumulate duplicate entries,
leading to multiple sessions being opened for the same SessionFactory. Consider
either clearing the list at the start of setHibernateDatastore or adding a
check to prevent duplicate additions.
##########
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:
If an exception occurs while opening or binding a session for an additional
datasource (e.g., lines 96-99), previously opened sessions in the loop won't be
cleaned up. Consider wrapping the session opening logic in a try-catch block
and, on failure, rolling back any sessions that were successfully opened before
re-throwing the exception. This prevents resource leaks when initialization
fails partway through.
```suggestion
List<SessionFactory> openedSessionFactories = new ArrayList<>();
try {
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);
openedSessionFactories.add(sf);
}
}
catch (RuntimeException ex) {
for (SessionFactory sf : openedSessionFactories) {
Object resource = null;
try {
resource =
TransactionSynchronizationManager.unbindResource(sf);
}
catch (IllegalStateException ignore) {
// Resource may already have been unbound; ignore.
}
if (resource instanceof SessionHolder) {
Session session = ((SessionHolder)
resource).getSession();
if (session != null) {
SessionFactoryUtils.closeSession(session);
}
}
}
throw ex;
```
##########
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:
If flushing one of the additional sessions throws an exception (line 136),
the remaining sessions in the loop won't be flushed or have their flush mode
set to MANUAL. Consider wrapping each session's flush logic in a try-catch
block to ensure all sessions are processed, accumulating exceptions and
re-throwing after all sessions are handled. This would match the robust cleanup
pattern typically needed for resource management.
```suggestion
RuntimeException firstException = null;
for (AdditionalSessionFactoryConfig config :
additionalSessionFactories) {
SessionFactory sf = config.sessionFactory;
SessionHolder additionalHolder = (SessionHolder)
TransactionSynchronizationManager.getResource(sf);
if (additionalHolder != null) {
Session additionalSession = additionalHolder.getSession();
try {
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();
}
}
catch (RuntimeException ex) {
if (firstException == null) {
firstException = ex;
}
else {
firstException.addSuppressed(ex);
}
}
}
finally {
additionalSession.setHibernateFlushMode(FlushMode.MANUAL);
}
}
}
if (firstException != null) {
throw firstException;
}
```
--
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]
