-
Clarification
-
Resolution: Done
-
Major
-
2.0-EDR1
-
None
So far CompletionStage is only mentioned in "10.5.1. Handling multiple exceptions thrown during an asynchronous event" and the Event.fireAsync() javadoc is too general. However, the CompletionStage itself does not define an unambiguous contract for its methods. E.g. what thread is used to execute a given callback? Or what's the "stage's default asynchronous execution facility"? I believe this is left on implementors. In Weld 3.0 Alpha we're using CompletableFuture under the hood, and its more concrete in this area, e.g.:
- Actions supplied for dependent completions of non-async methods may be performed by the thread that completes the current CompletableFuture, or by any other caller of a completion method.
So as a result, if an async delivery is finished before a sync dependent action is registered, the callback is executed in the caller thread:
event.fireAsync(new Message()).thenAccept((m) -> System.out.println("This might be executed in a caller thread or in a different thread!"));
And this might be confusing. Especially from the context propagation point of view. I think the spec should clarify the contract of a returned CompletionStage.
- is related to
-
-
- Resolved
-
[CDI-563] Event.fireAsync() - clarify the usage of the returned CompletionStage
Antoine Sabot-Durand (Inactive)
added a comment - What do you think we should add?
I don't like the wording "called in a new lifecycle contexts". What exactly does it mean? Maybe we should just rephrase this.
I'm not a big fan of this wording either. But it came from discussion with Jozef, and the fact that for the sake of non-denormalize the spec we shouldn't repeat what is said in 6.7:
The context associated with a built-in normal scope propagates across local, synchronous Java method calls. The context does not propagate across remote method invocations or to asynchronous processes.
Perhaps we could find a way to have a clearer phrase pointing to this assertion. It lets the possibility for a third party to create a normal scope supporting propagation.
Antoine Sabot-Durand (Inactive)
added a comment - What do you think we should add?
I don't like the wording "called in a new lifecycle contexts". What exactly does it mean? Maybe we should just rephrase this.
I'm not a big fan of this wording either. But it came from discussion with Jozef, and the fact that for the sake of non-denormalize the spec we shouldn't repeat what is said in 6.7 :
The context associated with a built-in normal scope propagates across local, synchronous Java method calls. The context does not propagate across remote method invocations or to asynchronous processes.
Perhaps we could find a way to have a clearer phrase pointing to this assertion. It lets the possibility for a third party to create a normal scope supporting propagation. 
Martin Kouba
added a comment - My favorite option as well . I'm just not sure where to add it in the chapter
I would add a new paragraph in 10.2.3. The Event interface... something like "The fireAsync() methods return a CompletionStage whose default asynchronous execution facility is specific to the container implementation."
What do you think we should add?
I don't like the wording "called in a new lifecycle contexts". What exactly does it mean? Maybe we should just rephrase this.
Martin Kouba
added a comment - My favorite option as well . I'm just not sure where to add it in the chapter
I would add a new paragraph in 10.2.3. The Event interface... something like "The fireAsync() methods return a CompletionStage whose default asynchronous execution facility is specific to the container implementation."
What do you think we should add?
I don't like the wording "called in a new lifecycle contexts". What exactly does it mean? Maybe we should just rephrase this. Antoine Sabot-Durand (Inactive)
added a comment - I'm not sure to follow him there, especially when CompletionStage has a dependency on CompletableFuture with the toCompletableFuture() method
It's kind of a strange dependency, intended probably as a compatibility/interoperation feature. However, implementations are not required to support this conversion.
I'm not sure, but I think we should, since it's the only way to access the get() method to allow user to wait for the outcome...
I don't think ForkJoinPool.commonPool() is a good fit for Java EE. But it's ok for Java SE.
+1
I'm for:
- having CompletionStage in the CDI API
Yes we also agree here.
- declaring explicitly that the stage's default asynchronous execution facility is not defined, in other words it is implementor and environment specific
My favorite option as well 
. I'm just not sure where to add it in the chapter
- declaring explicitly that the context propagation is not guaranteed for ANY execution (default execution, default async execution and custom async via a supplied Executor)
Regarding propagation, I thought that the mention in 10.5.3. Observer method invocation context, was explicit enough:
If the observer method is asynchronous, it is called in a new lifecycle contexts, a new transaction context but the same security context as the invocation of Event.fireAsync().
What do you think we should add?
Antoine Sabot-Durand (Inactive)
added a comment - I'm not sure to follow him there, especially when CompletionStage has a dependency on CompletableFuture with the toCompletableFuture() method
It's kind of a strange dependency, intended probably as a compatibility/interoperation feature. However, implementations are not required to support this conversion.
I'm not sure, but I think we should, since it's the only way to access the get() method to allow user to wait for the outcome...
I don't think ForkJoinPool.commonPool() is a good fit for Java EE. But it's ok for Java SE.
+1
I'm for:
having CompletionStage in the CDI API
Yes we also agree here.
declaring explicitly that the stage's default asynchronous execution facility is not defined , in other words it is implementor and environment specific
My favorite option as well . I'm just not sure where to add it in the chapter
declaring explicitly that the context propagation is not guaranteed for ANY execution (default execution, default async execution and custom async via a supplied Executor)
Regarding propagation, I thought that the mention in 10.5.3. Observer method invocation context , was explicit enough:
If the observer method is asynchronous, it is called in a new lifecycle contexts, a new transaction context but the same security context as the invocation of Event.fireAsync().
What do you think we should add? I'm not sure to follow him there, especially when CompletionStage has a dependency on CompletableFuture with the toCompletableFuture() method
It's kind of a strange dependency, intended probably as a compatibility/interoperation feature. However, implementations are not required to support this conversion.
Martin Kouba, you're probably the best person to know if this default Executor is the best for us or if we should use our own Executor...
I don't think ForkJoinPool.commonPool() is a good fit for Java EE. But it's ok for Java SE.
I'm for:
- having CompletionStage in the CDI API
- declaring explicitly that the stage's default asynchronous execution facility is not defined, in other words it is implementor and environment specific
- declaring explicitly that the context propagation is not guaranteed for ANY execution (default execution, default async execution and custom async via a supplied Executor)
What do you think?
Martin Kouba
added a comment - - edited I'm not sure to follow him there, especially when CompletionStage has a dependency on CompletableFuture with the toCompletableFuture() method
It's kind of a strange dependency, intended probably as a compatibility/interoperation feature. However, implementations are not required to support this conversion.
Martin Kouba, you're probably the best person to know if this default Executor is the best for us or if we should use our own Executor...
I don't think ForkJoinPool.commonPool() is a good fit for Java EE. But it's ok for Java SE.
I'm for:
having CompletionStage in the CDI API
declaring explicitly that the stage's default asynchronous execution facility is not defined , in other words it is implementor and environment specific
declaring explicitly that the context propagation is not guaranteed for ANY execution (default execution, default async execution and custom async via a supplied Executor)
What do you think? Antoine Sabot-Durand (Inactive)
added a comment - My first idea was to go directly for CompletableFuture in our API (methods return type, etc...). rhn-engineering-jharting raised concerns about using an impl where an API exist (I agreed with him) and features in CompletableFuture that we might not allow our users to have access to (I'm not sure to follow him there, especially when CompletionStage has a dependency on CompletableFuture with the toCompletableFuture() method).
So even if we won't expose CompletableFuture in our API for good practices, we won't reimplement what was nicely done in JDK8 and will use CompletableFuture under the hood. That's why I dont think we can say CDI will be an implementor of CompletionStage, we will be a user of CompletableFuture thru its main interface.
The only open question, IMO, is about default Executor used in CompletableFuture (based on fork/join if I'm not mistaken). mkouba@redhat.com, you're probably the best person to know if this default Executor is the best for us or if we should use our own Executor (CompletableFuture has methods to override default executor).
So, unless I missed something, either we specify the behavior from default Executor in completableFuture or define ours. The user will always have the possibility to use her own Executor with the dedicated fireAsync() method.
Antoine Sabot-Durand (Inactive)
added a comment - My first idea was to go directly for CompletableFuture in our API (methods return type, etc...). rhn-engineering-jharting raised concerns about using an impl where an API exist (I agreed with him) and features in CompletableFuture that we might not allow our users to have access to (I'm not sure to follow him there, especially when CompletionStage has a dependency on CompletableFuture with the toCompletableFuture() method).
So even if we won't expose CompletableFuture in our API for good practices, we won't reimplement what was nicely done in JDK8 and will use CompletableFuture under the hood. That's why I dont think we can say CDI will be an implementor of CompletionStage , we will be a user of CompletableFuture thru its main interface.
The only open question, IMO, is about default Executor used in CompletableFuture (based on fork/join if I'm not mistaken). mkouba@redhat.com , you're probably the best person to know if this default Executor is the best for us or if we should use our own Executor (CompletableFuture has methods to override default executor).
So, unless I missed something, either we specify the behavior from default Executor in completableFuture or define ours. The user will always have the possibility to use her own Executor with the dedicated fireAsync() method. Martin Kouba
added a comment - Ok, so first the CompletionStage API itself does not define the behavior completely. E.g. "default asynchronous execution facility" is left on implementors - and CDI is de facto an implementor. If we don't state anything it's unspecified. And I don't believe this part of the spec should be a place for experiments. Re context propagation - that's just the one of the clarifications I'm talking about. I think that if we don't specify anything, users would expect contexts to always work.
Martin Kouba
added a comment - Ok, so first the CompletionStage API itself does not define the behavior completely. E.g. "default asynchronous execution facility" is left on implementors - and CDI is de facto an implementor. If we don't state anything it's unspecified. And I don't believe this part of the spec should be a place for experiments. Re context propagation - that's just the one of the clarifications I'm talking about. I think that if we don't specify anything, users would expect contexts to always work. Antoine Sabot-Durand (Inactive)
added a comment - I'm not sure to get your point.
We are using API (and impl) provided by Java 8. Async event behavior won't be more confusing than this standard API.
Regarding context propagation as we said we won't support it, I don't think it'll be a problem
Antoine Sabot-Durand (Inactive)
added a comment - I'm not sure to get your point.
We are using API (and impl) provided by Java 8. Async event behavior won't be more confusing than this standard API.
Regarding context propagation as we said we won't support it, I don't think it'll be a problem 
+1