Hi David; Good catch.
Thanks; --Gurkan 2009/10/1 David Blevins <[email protected]> > Was looking at the observer code in NotificationManager and noticed that it > has a couple issues. Fortunately, they're far harder to explain than to > fix. > > The first issue is that the NotificationManager is repeatedly registered as > a javax.transaction.Synchronization and it holding a list of all the > transactional events it's received. It executes this list against the > interested Observers, firing the Observer.notify() method, when the > Synchronization.beforeCompletion and afterCompletion methods are called. > The gotcha is that the TransactionManager will call those methods as many > times as the Synchronization object is registered, which is currently once > per event per observer. > > So say in one Transaction there are 2 events fired and 3 > "BEFORE_COMPLETION" Observers. The NotificationManager will get registered > 6 times and will then get 6 Synchronization.beforeCompletion() invocations. > Each time it will fire off both events to all three Observers resulting in > each Observer getting a total of 12 Observer.notify(event) calls -- should > be just 2 calls per observer. > > The other issue is that the NotificationManager instance is multi-threaded. > In TransactionManager-world multi-threaded means that multiple transactions > could be executing against it concurrently. Yes, the NotificationManager > uses a thread-safe Set for collecting events, but that doesn't protect > against two transactions (threads) intermittently adding Observer events to > that one collection. So observers may get events from someone else's > transaction. > > The fix is easy. We don't need to keep a list and just need to register > one javax.transaction.Synchronization per event. And really we should be > using the > TransactionSynchronizationRegistry.registerInterposedSynchronization method > rather than Transaction.registerSynchronization. Main reason is that it > guarantees the observers will get a chance to work with things like the > EntityManager *before* the EntityManager itself gets its own Synchronization > callbacks and flushes the data for the last time, etc. For now we can > probably tuck the registration part under the covers of the > TransactionService interface and leave it to the implementor to use the > registration technique they want. > > > Will take a crack at fixing this tomorrow. > > > -David > > -- Gurkan Erdogdu http://gurkanerdogdu.blogspot.com
