| After reviewing your test case, the first thing which stood out to me was how you handled closing the EntityManager. Your code does this:
finally {
entityManager.close();
}
One could argue that potentially the EntityManager was never constructed or opened in the try-block for some reason. With that in mind, I would say a better finally-block would be this:
finally {
if ( entityManager != null && entityManager.isOpen() ) {
entityManager.close();
}
}
This has a positive impact on your situation because it guarantees that the AfterTransactionCompletion callbacks fire, thus avoiding the memory leak you describe and illustrate in your test case. The reason is because the #isOpen() method performs some transaction management checks that notice that the callbacks were delayed by the reaper thread and must execute on the original calling thread. Its this call which effectively forces them to fire thus clearing the AuditProcess queue for you. With that said, this will throw a HibernateException because the underlying SynchronizationCallbackCoordinatorTrackingImpl#processAnyDelayedAfterCompletion will be called and presently informs the original calling thread of the following after executing the AfterTransactionCompletion callbacks.
throw new HibernateException( "Transaction was rolled back in a different thread!" );
What you could then do is handle that exception yourself if you anticipate it so it doesn't present an issue as follows:
finally {
try {
if ( entityManager != null && entityManager.isOpen() ) {
entityManager.close();
}
}
catch ( PersistenceException e ) {
}
}
I could discuss this with the team to see if there are any concerns with us adding the same type of transaction management checks from #isOpen() to #close(). If there is no issue, that would atleast allow those after-completion callbacks to fire to avoid the memory leak you described. But that will still imply a change in your finally block to handle the PersistenceException for your use case. |