Awesome work! Thanks;
--Gurkan 2009/10/7 David Blevins <[email protected]> > 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 >> > > -- Gurkan Erdogdu http://gurkanerdogdu.blogspot.com
