Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add @Asynchronous Annotation #43

Closed
Pandrex247 opened this issue Apr 5, 2018 · 12 comments
Closed

Add @Asynchronous Annotation #43

Pandrex247 opened this issue Apr 5, 2018 · 12 comments
Labels
enhancement New feature or request Type: New Feature
Milestone

Comments

@Pandrex247
Copy link

Pandrex247 commented Apr 5, 2018

EJB and MicroProfile Fault Tolerance each have their own Asynchronous annotation. This seems like the kind of thing that should belong under the concurrency spec, with the other specs expanding upon it as required.

At a base level, it would simply mean that a method gets executed using a separate thread (with the method returning a Future). This could then be extended in a number of ways, such as specifying the name of a preconfigured ExecutorService to use (rather than always using the default one).

@smillidge smillidge added enhancement New feature or request Type: New Feature labels Apr 9, 2018
@smillidge
Copy link
Contributor

In my view this would essentially execute the method using the Managed Executor submit api. The annotation name could specify the JNDI name of the Executor to use with the standard default executor used if no other is specified.

@njr-11
Copy link
Contributor

njr-11 commented Nov 6, 2019

The MicroProfile FaultTolerance @Asynchronous allows return types of either CompletionStage or Future, with CompletionStage being preferred. Refer to the following JavaDoc.

FYI - There is also an issue open under MicroProfile FaultTolerance with some discussion that @Asynchronous might be better placed elsewhere. In the issue, they were suggesting moving it to MicroProfile Concurrency/Context Propagation, which was before Jakarta specs got started up again. This could be a good opportunity to offer to accept it into Jakarta Concurrency. eclipse/microprofile-fault-tolerance#415

@smillidge smillidge added this to the 3.0 milestone Mar 11, 2021
@tkburroughs
Copy link
Member

The EJB @Asynchronous allows return types of either void or Future<V>, where V is the result value type. void is useful for scenarios where a result is never used.

The javadoc for the MicroProfile FaultTolerence @Asynchronous is very specific to microprofile, and there appear to be some differences between the EJB and microprofile versions. Would be interesting to see if a common @Asynchronous annotation could be defined that would support both specifications. Some differences I see are:

  • EJB has some unique behavior around Future.cancel(boolean). false will only cancel if it hasn't started yet; true will only make the attempt visible to the asynchronous thread via the EJB SessionContext.wasCancelled() method, so the asynch thread can check that periodically and abort if it might be long running.
  • EJB supports an XML equivalent of the annotation, so if we would like the new @Asynchronous to work with EJB, then we'd want to define that, or at leaset allow other specifications to provide a way to do that
  • EJB currently doesn't require specific deployment exceptions for anything; may want to allow specifications to ignore the exception is not correct.
  • EJB would want to be allowed to ignore the @Asynchronous annotation of metadata-complete="true" is specified
  • EJB version of @Asynchronous does not contain @InterceptorBinding; would have to consider the impact of that on EJB.

@njr-11
Copy link
Contributor

njr-11 commented May 7, 2021

Another difference is that MicroProfile FaultTolerance @Asynchronous allows (and recommends) returning CompletionStage. This will typically be a CompletableFuture, so also an implementation of Future. If we add this to Concurrency, I would expect to see CompletableFuture used as the return type, and it should tie in quite well with all of the support we are adding to the Concurrency spec around completable futures backed by managed executor service.

That said, the Future.cancel operation that Tracy points out has inconsistencies and is a problem that we will need to address. I can see two ways of designing this, and they have different implications with respect to the meaning of cancel.

The first would be as MicroProfile/EJB have done where the invoker of the asynchronous method gets a [Completable]Future for the [Completable]Future that is created and returned by the method itself. It doesn't get the same [Completable]Future instance, because, being asynchronous, the method doesn't even have the opportunity to create it yet. Thus, there are two [Completable]Future instances involved, with the invoker having what is essentially a copy of the other. In this case, it would be wrong for cancellation (or forced completion by any other means -- there are several) of the invoker's copy (which is a dependent stage in CompletableFuture terms) to have any impact on the original. That would violate how completable futures work, and is likely the reason why MicroProfile FaultTolerance stays out of the way here and doesn't offer a cancel operation. In terms of CompletableFuture, a dependent stage can be impacted by the completion of a stage upon which it depends (the one returned by the method impl) but not the other way around. If you cancel (or forcibly complete by other means) a dependent stage, it impact the stage itself, but any stage that it depended on must be unimpacted. If cancel of the stage created by the asynchronous method impl is important, there would need to be a dedicated API specifically for that purpose, not a reuse of the dependent stage which already has a different meaning of its own.

@Asynchronous
CompletableFuture<Integer> doSomethingAsync() {
   if (!Asynchronous.Result.isCancelled()) {
       ...
       return CompletableFuture.completedFuture(100);
   }
}
...
dependentStageFuture = obj.doSomethingAsync();
dependentStageFuture.cancel(); // only cancels the dependent stage
// awkward API would be needed to cancel the asynchronous method:
Asynchronous.Result.cancel(dependentStageFuture);

The second option here -- if we wanted to allow a cancel on the [Completable]Future to actually flow through, would be to design it such that the Asynchronous method doesn't create its own [Completable]Future, but instead obtains an incomplete Future via some API, Asynchronous.Result.getFuture() ? in which case both the invoker and the asynchronous method impl would share the same instance. This still wouldn't stop the Asynchronous method from running though. It would only serve as a signal that it has already been cancelled or forcibly completed, which the Asynchronous method could react to by ceasing its own execution, if it notices. The whole concept of cancel doesn't seem all that valuable unless I'm missing something. But I thought I'd at least mention it as a possibility.

@Asynchronous
CompletableFuture<Integer> doSomethingAsync() {
   CompletableFuture<Integer> future = Asynchronous.Result.getFuture();
   if (!future.isDone()) { // could check again throughout method if we want
       ...
       if (future.complete(100))
           return future;
   }
}
...
future = obj.doSomethingAsync();
future.cancel(); // cancels the shared instance

@njr-11
Copy link
Contributor

njr-11 commented Aug 17, 2021

Something else to consider as an alternative to @Asynchronous is that you inadvertently get a subset of the behavior with the new CompletableFuture support that is (hopefully) going in to ManagedExecutorService.

In this case, you don't need any annotation at all if the methods don't have parameters, for example,

    static int myAsyncMethod() {
        return 50;
    }

    int anotherAsyncMethod() {
        return 100;
    }

which can be invoked as follows:

        future = executor.supplyAsync(MyClass::myAsyncMethod);
        future = executor.supplyAsync(myClass::anotherAsyncMethod);

And if ManagedExecutorService were further enhanced with a couple of additional trivial methods,

        public <R, T> CompletableFuture<R> applyAsync(Function<T, R> fn, T param1) ;
        public <R, T, U> CompletableFuture<R> applyAsync(BiFunction<T, U, R> fn, T param1, U param2) ;

then you would also be able to take methods with 1 or 2 parameters like these,

    static int cube(int param1) {
        return param1 * param1 * param1;
    }

    int add(int param1, int param2) {
        return param1 + param2;
    }

and get the equivalent of async methods via

        future = executor.applyAsync(MyClass::cube, 4);
        future = executor.applyAsync(myClass::add, 5, 7);

but to go any further, you would need more functional interfaces (and equivalent ManagedExecutorService apply methods) taking higher numbers of parameters.

I really like the straightforwardness and consistency of this approach (the method to be run asynchronously is just a normal method with its natural signature and return value), but it's a pretty big problem that there is no way to allow for a variable number of parameters, so it's too limited in that way. I thought I would bring it up though in case someone does see how to solve that problem.

@njr-11
Copy link
Contributor

njr-11 commented Sep 10, 2021

I'm doing some initial experimentation with this in the Open Liberty repo under OpenLiberty/open-liberty#18484 which currently has a draft Async annotation class with some initial JavaDoc. It looks promising thus far. Once I get further along on it, I'll create a pull in the spec repo.

njr-11 added a commit to njr-11/concurrency-api that referenced this issue Sep 20, 2021
@njr-11
Copy link
Contributor

njr-11 commented Sep 20, 2021

Pull #147 is available for review. It attempts to address a variety of usage patterns and also ties in with the managed CompletableFuture support in Concurrency 3.0. It doesn't provide an alternative to every pattern that EJB has, meaning we would end up with a situation where the existing Asynchronous remains for EJB to use versus this new Async annotation for everywhere else.

@smillidge
Copy link
Contributor

Can we pick up the remaining EJB functionality?

@njr-11
Copy link
Contributor

njr-11 commented Oct 4, 2021

Can we pick up the remaining EJB functionality?

Looking through Tracy's list of differences with EJB for the remaining EJB functionality,

  • EJB has a special form of cancel that "will only make the attempt visible to the asynchronous thread via the EJB SessionContext.wasCancelled() method, so the asynch thread can check that periodically and abort if it might be long running." EJB also has normal cancel that we are consistent with. I wouldn't change our cancel over to the special form, but I think users who want the special notification mechanism could easily achieve it just by supplying an AtomicBoolean as a method parameter,
  @Async
  CompletableFuture<String> doSomethingAsync(AtomicBoolean stop) {
      for (...)
          if (stop.get())
              throw new CancellationException("Stopped by request from user");
          else
              ...
  }
  • "EJB supports an XML equivalent of the annotation". At first I thought CDI beans.xml might let you do the equivalent of annotions, but it appears not to. It might actually be helpful to have this limitation in our case because it will force the asynchronous annotation to appear on the method itself, making it very explicit what is asynchronous and what is not. So I would lean toward not matching this capability of EJB.

  • "EJB currently doesn't require specific deployment exceptions for anything". Our current proposal doesn't require deployment exceptions either, which was mainly due to our error conditions (like lack of availability of a managed executor JNDI name) not being evident until run time. There is some discussion, however, which just came up about having a static error condition for misplacing the Async annotation be raised as a deployment exception. I would be fine with having that static error detected at runtime instead for better consistency. At runtime, it will probably generate a more helpful stack anyway, identifying the exact package and class name that is in error.

  • The final 2 points from Tracy's list are discussing aspects which are only relevant within EJBs and not for our discussion of non-EJB scenarios matching what EJB asynchronous can do.

Hopefully this gets us close enough. Let me know if you have any other ideas.

@tkburroughs
Copy link
Member

From my list of difference above, only the first is really a difference in capability..... the rest are all concerns related to the question: can the new @Async support be applied to EJB.... and perhaps how it would behave in a WAR module.

For example:

  • EJB would want to be allowed to ignore the @Async annotation if metadata-complete="true" is specified

Both EJB and WAR modules have this metadata-complete setting, which turns off all annotation processing for EJBs and servlets...... it would seem the specification for the new @Async annotation may want to address how it relates to the metadata-complete setting for EJB and WAR modules..... and is that behavior allowed to be different if the annotation is declared on a CDI bean in an EJB jar vs an EJB in the same EJB jar.

However, I see the current proposal is that the new @Async may only be applied to CDI managed beans.... and by that I presume that you don't intend for it to be applied to EJB.... in which case the all but the first point could perhaps be ignored. Be aware that some may interpret a CDI managed bean to include EJB, as some places document that an EJB is just a special type of CDI managed bean. JSF also has a managed bean, which is another special case of a CDI managed bean. Someone more familiar with the CDI specification may want to provide the proper wording to clarify specifically which types of CDI managed beans the annotation may be applied to... as I don't think we'd want anyone to believe it could be applied to an EJB if the EJB spec isn't going to be updated to support it.

njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 4, 2021
njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 4, 2021
@Emily-Jiang
Copy link

Emily-Jiang commented Oct 5, 2021

However, I see the current proposal is that the new @Async may only be applied to CDI managed beans.... and by that I presume that you don't intend for it to be applied to EJB.... in which case the all but the first point could perhaps be ignored. Be aware that some may interpret a CDI managed bean to include EJB, as some places document that an EJB is just a special type of CDI managed bean. JSF also has a managed bean, which is another special case of a CDI managed bean. Someone more familiar with the CDI specification may want to provide the proper wording to clarify specifically which types of CDI managed beans the annotation may be applied to... as I don't think we'd want anyone to believe it could be applied to an EJB if the EJB spec isn't going to be updated to support it.

@tkburroughs has a good point here. I agree with the above comments. We should just say this annotation applies to CDI beans that are not EJBs as EJBs are CDI beans. We should just leave EJB spec at rest and not to touch it.

njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 6, 2021
njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 8, 2021
njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 8, 2021
njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 8, 2021
njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 8, 2021
njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 14, 2021
njr-11 added a commit to njr-11/concurrency-api that referenced this issue Oct 14, 2021
@njr-11 njr-11 closed this as completed Jan 13, 2022
@arjantijms
Copy link
Contributor

@tkburroughs

JSF also has a managed bean, which is another special case of a CDI managed bean.

I know I'm late to the party and the issue is closed already, but just for reference; the above is not correct on two accounts.

JSF no longer has its own managed beans. They were deprecated in 2.3, and removed in 4.0. Also, JSF managed beans were never a special case of a CDI managed bean. They were a totally different kind of bean, that only (like Servlets) emulates a few capabilities regarding injection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Type: New Feature
Projects
None yet
Development

No branches or pull requests

6 participants