asynchronous methods annotation

[njr-11]

Proposal for #43 asynchronous methods annotation. This ties into the new support for managed CompletableFuture/CompletionStage instances which includes being able to run all dependent stage actions (including the asynchronous method) with the requesting thread context.

Signed-off-by: Nathan Rauh [email protected]

[smillidge]

Couple of questions otherwise I think it is a nice API. Are we saying nothing about non-CDI beans eg JAX-RS Resources or are they explicitly not supported?

[smillidge]

Should it also turn all void methods asynch?

[njr-11]

I was a bit hesitant about even allowing the asynchronous methods annotation at class level at all, except that it is useful there for the CDI extension to annotate a class as the interceptor.
Once we have it at class level, then there's a risk that the user will end up with methods inadvertently becoming asynchronous that they didn't expect to be. For example, method String getName() isn't asynchronous (due to String return type), and if a user adds void setName(String name) it likely won't even cross their mind that the method would be asynchronous, which could end up causing intermittent bugs due to the unexpected parallelism. The opposite could happen too, but that seemed both less likely and less severe.
I wasn't sure what the best approach is here, so I'm glad to have others thinking through it as well.
Another option is to have the spec state that applications are not to specify the asynchronous methods annotation at class level (this is only for the CDI extension to do in defining an interceptor) and that applications must only define it at the method level. This could be easily enforced by the interceptor and remove the ambiguity over which methods do/don't become asynchronous, at the trade-off of possibly being confusing because the annotation would still have ElementType.TYPE in its @Target.
What is your preference?

[smillidge]

I think not allowing a class level annotation is probably the safest solution from the perspective of the developer using the api in that side effects are reduced. Do you think we can detect and specify a deployment exception should be thrown if application code marks it at the class level?

I know people complain about verbosity but personally I think explicitly marking methods with asynchronous would be best practice anyway and easier for anybody having to maintain the application code.

[njr-11]

I know people complain about verbosity but personally I think explicitly marking methods with asynchronous would be best practice anyway and easier for anybody having to maintain the application code.

Since we both agree that limiting to method level is the best practice, let's go with that approach. It will simplify the API and make application code more readable and explicit about the behavior it is getting.

I think not allowing a class level annotation is probably the safest solution from the perspective of the developer using the api in that side effects are reduced. Do you think we can detect and specify a deployment exception should be thrown if application code marks it at the class level?

I know for sure that an interceptor can at least detect it at runtime and raise an error (such as UnsupportedOperationException) upon the attempt to invoke a method on the invalidly annotated class. I'll need to look into whether there is a way to detect it upon deployment to raise a deployment exception then.

[smillidge]

This bit is unclear to me. If the Method is marked jakarta.transaction.Transactional.TxType.REQUIRES_NEW and Asynch is the asynch invocation in a new transaction, my reading is that is implied but not explicitly stated? Also what happen if it is marked with REQUIRED

[njr-11]

Good point - @Transactional with REQUIRES_NEW will require the method to run in a new transaction, and we should be explicit about that. I'll add that in for clarity after we get the rest of this figured out. Regarding what happens with REQUIRED, I documented that on the @Async API, All other transaction attributes must result in {@link java.lang.UnsupportedOperationException UnsupportedOperationException} upon invocation of the asynchronous method. but I forgot to also include that here. Thanks for spotting this - I'll plan to get that added in as well.

[smillidge]

I have a question on the name of this should we use full Asynchronous or Asynch with 'h'?

[njr-11]

I have a question on the name of this should we use full Asynchronous or Asynch with 'h'?

It doesn't make any difference to me - I'd be happy with any of the 3 options, and lacking other input had just used the shortest. What is your preference? I'd be happy to change it.

[njr-11]

Are we saying nothing about non-CDI beans eg JAX-RS Resources or are they explicitly not supported?

Thanks for bringing up the question of non-CDI beans. I checked with @andymc12 who works on the JAX-RS specification about their strategic direction going forward, and he informed me that "the JAX-RS community is moving to where all JAX-RS resources and providers will be CDI beans, but not until JAX-RS 4.0 (currently slated for EE11). For JAX-RS 3.1 (EE10), the non-CDI approach is deprecated, but still available." From the JAX-RS perspective then, aligning with CDI managed beans seems the right strategy.

[smillidge]

I have a question on the name of this should we use full Asynchronous or Asynch with 'h'?

It doesn't make any difference to me - I'd be happy with any of the 3 options, and lacking other input had just used the shortest. What is your preference? I'd be happy to change it.

Naming is the hardest thing! To be honest I don't have a strong preference. EJB and MicroProfile uses the full Asynchronous spelling whereas servlet uses asyncSupported and JAX-RS AsyncResponse

[tkburroughs]

@Async has an advantage that it would be unique from EJB and MicroProfile; there won't be issues with editors automatically adding the wrong import. If not using @Asynchronous, then I prefer @Async without the h.

[njr-11]

@smillidge - commit 6f77208 makes updates per the code review comments for limiting to methods only and adding more doc around interoperability with @Transactional. I still need to track down the answer regarding proper terminology for "CDI managed bean" that doesn't imply EJBs, and haven't made any updates to the annotation name yet. I see Tracy voted for Async, which I'll note also aligns well with the many methods of CompletableFuture/CompletionStage of the form runAsync, supplyAsync, thenApplyAsync, ...

[njr-11]

Commit 414346e adds the language around prohibiting use from EJB, as well as JSF managed beans (non-strategic and lacking support for CDI interceptors) and MicroProfile Asynchronous (duplicate function and is nonsensical to annotate with both at the same time).

As far as I am aware, review comments have all been addressed now.

[njr-11]

rebased onto master and added commit d027c29 to fix checkstyle issues

[arjantijms]

I really like that this one has finally incorporated the executer/pool to run the method on. I would need to look it up, but from memory it might just well be the 10th anniversary since this was pitched back in the days ;)

A tiny bit of feedback regarding the following:

The CDI managed bean must not be an EJB or JSF managed bean,

JSF doesn't exist anymore in Jakarta 9+, it's Faces now ;) Same holds for EJB which is now Enterprise Beans.

Also, a Faces managed bean could never be a CDI managed bean at the same time. Finally and probably most importantly, in Jakarta EE 10/Faces 4, Faces managed beans don't exists anymore. They have been fully removed (pruned) from the specification. Faces now 100% relies on CDI managed beans, and CDI managed beans only.

[njr-11]

A tiny bit of feedback regarding the following:

The CDI managed bean must not be an EJB or JSF managed bean,

JSF doesn't exist anymore in Jakarta 9+, it's Faces now ;) Same holds for EJB which is now Enterprise Beans.

Also, a Faces managed bean could never be a CDI managed bean at the same time. Finally and probably most importantly, in Jakarta EE 10/Faces 4, Faces managed beans don't exists anymore. They have been fully removed (pruned) from the specification. Faces now 100% relies on CDI managed beans, and CDI managed beans only.

@arjantijms thanks for spotting that and clarifying the changes in Jakarta Faces!
I added commit 47810ac with corrections.