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
