Ok, I've committed that change.
In the process of doing so it became pretty clear why the
javax.enterprise.event.Observer interface was removed. It simply
isn't needed as the @Observes annotation can capture that just fine.
Internally we should just be wrapping these as
javax.enterprise.inject.spi.ObserverMethod instances and the
NotificationManager should be reworked once again to hold a collection
of them instead of the ObserverWrapper inner class. That would
actually simplify the NotificationManager code even more.
I've filed this as https://issues.apache.org/jira/browse/OWB-140
More generally it seems we're missing quite a bit of the overall
ObserverMethod contract such as firing ProcessObserverMethod events.
Going to pick through that part of the spec some more and come up with
some jiras.
-David
On Oct 1, 2009, at 1:30 AM, Gurkan Erdogdu wrote:
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
