[cdi-dev] [JBoss JIRA] (CDI-499) Firing events asynchronously

[Martin Andersson (JIRA)]

    [ https://issues.jboss.org/browse/CDI-499?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13060395#comment-13060395 ] 

Martin Andersson commented on CDI-499:
--------------------------------------

h2. {{fireAsync()}}

"Discussion is open *but*".. sounds like the discussion is not open =) Josés idea about an {{Executor}} is excellent. We talk about ONE method right? Easy to add, huge impact on the world. If you want "basic tasks", then why do we want to add an asynchronous feature to begin with? Josés point about being true to the nature of asynchronous Java API:s are undoubtedly a good one. Either don't implement the feature, or go all the way and deliver a great product. Time to step up our game if we want to compete with other technologies available on the market.

I don't think {{asyncSupported()}} should default to {{false}}. It should default to {{true}}.

Firstly, backward compatibility must not stand in the way of technology advances. That's kind of why the major release version become a number "2". And if I'm thinking clearly here, then {{true}}, which is without doubt (?) the most natural fit, does not break backward compatibility. Legacy code already call {{Event.fire()}}. If I write new and modern code and call {{Event.fireAsync()}}, then isn't it it my responsibility to make sure that if I use old event container types and thus target old observer methods, that the observer can handle asynchronicity?

To put things in another perspective: An observer method that does *not* support asynchronicity is knowledgeable and dependent on who fired the event, how and where. He is dependent on a particular context. That sounds like a design smell to me. But, the one firing the event should have a saying if he want the call to be blocking or not. But imagine the confusion a default false would produce: calling {{fireAsyn()}} is most likely to actually block. Depends on the observer methods! Simply put it, the developer must scan through the entire code base every time to make sure his asynchronous request really will be honored. Even worse, he can never be sure that will always be the case. 5 minutes later, another developer add an observer method and will *by default* make all unknowingly asynchronous events of this type in the system turn synchronous. At best, this will have such an enormous impact on user experience and create large enough bottlenecks to be discovered. At worst, the application will slowly die a painful death as the number of parallell tasks going down the serialized lane increase.

Simply put it, don't fix what's not broken. Letting the observer methods have a saying smells bad. Having a method be called {{fireAsync()}} but who in fact work nondeterministically smells even worse and can create all kinds of drama. I think {{asyncSupported}} should be dropped. If it must be in place, then let it by default be {{true}}. If so, it will be the one implementing the observer method that might destroy the application, not the unknowing poor guy who wrote the world's most beautiful code, thinking a method called {{fireAsync()}} actually work as advertised.

As an example, I don't use {{@Asynchronous}} from EJB because of the nondeterministic behavior. I am not guaranteed that the container actually run my task. He might reject the task. And the way exception handling work in EJB (which is contrary to the spec's own design goal), I can never be sure that my portable application code can determine why the EJB call crashed. So basically, I must use {{ManagedExecutorService}} and be prepared to catch a {{RejectedExecutionException}}. So, EJB asynchronicity is dead and of no use for serious application code that want to be portable. Please don't make the same mistake with CDI. Oh you should also specify that fireAsync() might throw RejectedExecutionException?

h2. {{asyncSupported}}

To continue my "ranting" - or improvement ideas =) .. I see what might be a problem if asynchronous observer methods are prioritized, submitted before synchronous observers are called. Today, if a non-transactional observer method crash, then the event processing is aborted - no more observers called. But what if we already submitted asynchronous tasks to a queue somewhere, are they aborted? Are they guaranteed to be aborted if execution of the tasks has not begun when a synchronous observer crash? If we want to keep the "feature" that an observer method should be able to stop propagation, then asynchronous observers should be called ??after?? all synchronous observers has been called. OR be very clear in JavaDoc and in the new specification that it is *not defined* whether or not synchronous observers that crash stop event propagation to asynchronous observers. I understand that it is more pleasing to have the asynchronous tasks be submitted as fast as possible. And if I were the one designing the spec, then observer method crashes would not stop propagation. Instead, {{EventMetadata}} should have a method for that. To elaborate a bit on the backward compatibility, if CDI 2 really is supposed to be a new major release, we should take every chance we have to make improvements. Suppressed exceptions and say {{EventMetadata.stopPropagation()}} would be such a thing. Best of both worlds.

Thank you all for you hard work. Asynchronocity is a major improvement. Let's go all the way and put together a killer product =)

> Firing events asynchronously 
> -----------------------------
>
>                 Key: CDI-499
>                 URL: https://issues.jboss.org/browse/CDI-499
>             Project: CDI Specification Issues
>          Issue Type: Feature Request
>          Components: Events
>    Affects Versions: 1.2.Final
>            Reporter: Antoine Sabot-Durand
>
> We should allow a way to fire event asynchronously. This mechanism should leverage new async API in JDK8 especially the {{CompletionStage}} interface.
> Our proposal is: 
> h2. 1. Add {{fireAsync()}} method to {{Event}} and {{BeanManager}}
> Signature of the method on {{Event<T>}} would be
> {code:java}
> <U extends T> CompletionStage<U> fireAsync(U event);
> {code}
> Signature on {{BeanManager}} would be
> {code:java}
> <T> CompletionStage<T> fireAsyncEvent(T event, Annotation... qualifiers)
> {code}
> h2. 2. Add an {{asyncSupported()}} member to {{@Observes}}
> For backward compatibility reason the possibility to invoke an observer asynchronously should be let to the observer (legacy observers should be called synchronously). We propose to add the boolean {{asyncSupported()}} member with the {{false}} default value to support this backward compatibility aspect.
> So to be notified asynchronously an observer should have {{asyncSupported}} member to true. otherwise it will be called synchronously.
> h2. 3. Observer bound to a transaction phase
> these observer will be invoked in the right transaction phase but asynchronously
> h2. 4. Event Ordering
> Should we decide to add events ordering in CDI 2.0, the order will be keep in asynchronous observer notification. If there are a mix of synchronous and asynchronous observer, asynchronous will be called first in order, then synchronous in their order (async has priority on sync).
> h2. 5. Event state (payload mutability)
> We'll keep payload mutability with async events (but should explicitly specify it). That means  that we should guarantee the event state consistency between observers and in case of ordered observers the fact that observer N+1 get the event state at the end of observer N.  



--
This message was sent by Atlassian JIRA
(v6.3.11#6341)