Invoker support for asynchronous methods

[Azquelt]

Is your feature request related to a problem? Please describe.

Invokers cannot be used to portably call asynchronous methods. This would prevent Invokers being used to call Jakarta REST resource methods which execute asynchronously and return CompletionStage in a safe way without relying on non-spec features of the CDI container in use.

Background

Invokers allow the target instance and arguments for the method being invoked to be looked up from CDI. It looks up the bean matching the type and qualifiers, uses that as the instance or argument, calls the method and then destroys the reference afterwards.

For dependent scoped beans, this means that the bean will live for the duration of the method invocation and then be destroyed.

This is a problem if the method represents an asynchronous operation which may still be ongoing when the method returns and still require access to the beans used as the instance or arguments.

The spec [does allow][spec-impl-specific] for implementation-specific solutions to this problem, but such solutions are not portable.

Additionally, some practical considerations I think are important here:

• this only affects @Dependent scoped beans
  • normal-scoped beans will have their lifecycle tied to their context. The reference injected will be a proxy object which will delegate to the bean instance from the current context.
• some scopes (including @RequestScoped) may not be usable for asynchronous use anyway
• as far as I'm aware, the only consequence of destroying a @Dependent scoped bean is that any @PreDestroy methods of that bean or its dependents will be run. If there are no pre-destroy lifecycle callbacks, there may not be an issue with continuing to use the bean instance after it's been destroyed.

Describe the solution you'd like

An API is added which allows the looked up bean references to remain valid until after the asynchronous operation ends. There are various options for what this API may look like discussed in #749 and #768.

Describe alternatives you've considered
TODO add options discussed previously

Additional context
Add any other context or screenshots about the feature request here.

[manovotn]

Added CDI 5.0 milestone so we do not lose track of this.

[Azquelt]

Notes from call 11 March:

Example uses:

• JAX-RS resource methods
  • returning CompletionStage
  • accepting an AsyncResponse argument
• Kotlin suspend methods

We could add something onto the invoker builder, but it needs to be able to replace parameters or the return type so that some kind of callback can be registered to trigger the destruction of dependent objects.

[manovotn]

This hasn't really seen any interest/commotion since the proposal.
@Azquelt was this blocking JAX-RS specification? Do they expect it for 5.0?

[Azquelt]

I think it blocks JAX-RS using invokers to call all rest resource methods, since they support asynchronous operation.

Personally, I think this issue precludes using invokers for any portable implementation of a framework that supports asynchronous execution, so I would like to fix it.

I did have some suggestions on the original pull request. I'll look back at those and make a proposal.

I know there were originally concerns about making an API that users could call incorrectly, but at the moment I think we're in a worse state of having an API that's impossible to call correctly if you want to do asynchronous things.

[Azquelt]

I think we should add this method to InvokerBuilder:

InvokerBuilder<T> withAsyncHandler(Class<? extends InvokerAsyncHandler> handlerClass);

And then define InvokerAsyncHandler like this:

public interface InvokerAsyncHandler<T, R> {
    
    /**
     * Calls an Invoker, which starts an asynchronous operation, and ensures that {@code cleanup} is called when the asynchronous operation completes.
     * <p>
     * It's expected that implementations will register a callback with the asynchronous framework to call {@code cleanup} once the operation completes.
     * <p>
     * The implementation must ensure that {@code cleanup} is called even if the invoked method throws an exception or the asynchronous operation fails.
     * <p>
     * For example, an async handler for a {@link CompletionStage} might look like this:
     * <pre>{@code  try {
     *     CompletionStage<?> result = invoker.invoke(instance, parameters);
     *     result.handle((r, t) -> {
     *         cleanup.run();
     *         return null;
     *     });
     *     return result;
     * } catch (Throwable t) {
     *     cleanup.run();
     *     throw t;
     * }
     * }</pre>
     * 
     * @param invoker an invoker, to invoke the method
     * @param instance the instance being invoked, may be {@code null} if the instance is looked up from CDI
     * @param parameters the array of parameters to the invoker
     * @param cleanup a runnable to release any beans looked up as part of the invocation call
     * @return the return value from calling {@code invoker}
     */
    public R handleAsyncInvocation(Invoker<T, R> invoker, T instance, Object[] parameters, Runnable cleanup) throws Exception;
}

I don't really like that it makes InvokeAsyncHandler basically an interceptor (with one additional cleanup parameter), but I think it's necessary to allow the async handler to wrap either a parameter or the return value if necessary.

In addition, I think we need to provide a way for the InvokerAsyncHandler to introspect the method. IIRC for build compatible extensions we can't have the user provide the instance themselves so I was thinking that we should allow it to have a constructor that accepts a single MethodInfo parameter (and also allow AnnotatedMethod in CDI full).

The implementation would need to verify that the values for T and R of the provided class are valid for the instance and return type of the method.

[Azquelt]

I've a WIP of the API here: main...Azquelt:cdi:async-invoker
And of some TCK tests here: jakartaee/cdi-tck@master...Azquelt:async-invokers

[Ladicek]

In my opinion, we shouldn't require users to specify that they want method invocations to be asynchronous. It is relatively straightforward to determine whether a method should be called asynchronously -- typically by looking at the return type, but there are at least 2 situations where one has to look at parameter types (JAX-RS asynchronous methods, which have a @Suspended AsyncResponse parameter, and Kotlin suspend functions, which have last parameter of type Continuation). If we determine the method is asynchronous, we should demand that it is invoked asynchronously.

In other words, we should not define an API that all users have to use. We should define an SPI that containers should implement . It is unfortunate that it's not enough to look at the return type, because that would make everything a lot easier, but such is life.

Thinking about it, it seems we can design a minimal API that doesn't give access to the entire invocation, just allows decorating the return value or the argument value. Something like:

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
public @interface WrapsReturnValue {
}

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
public @interface WrapsArgumentValue {
}

public interface AsyncInvocation<T> {
    public T wrap(T original, Runnable completion);
}

Implementations of AsyncInvocation are discovered by ServiceLoader. The must have a zero-param constructor. They must be stateless (and therefore thread-safe). Their lifecycle is not specified; the container is permitted to instantiate just one object and use it for all invocations, or instantiate a new object for each invocation, or whatever else it wants. The implementation class must be annotated either @WrapsReturnValue or @WrapsArgumentValue, otherwise deployment fails.

Now the important part: how do we match the method return type (or parameter type) to the implementations of AsyncInvocation? By looking at the declaration of the first parameter of wrap(), especially on its type but also on its annotations. This can possibly be done during deployment (and so at build time in case of build-time implementations). Then, at runtime, the wrap() method is called on the return value or the corresponding argument value. If wrap() is called on the return value, the result of wrap() is returned from invoke() instead. If wrap() is called on an argument value, the result of wrap() is passed as the corresponding argument to invoke() instead.

EDIT: alternatively, we could say that for @WrapsReturnType, we match based on return type of wrap(), and for @WrapsArgumentType, we match on the type of the first parameter. Either works for me.

For example, for return type of CompletionStage:

// annotated `@WrapsReturnValue` and first parameter of `wrap()` is of type `CompletionStage`,
// so we match all target methods that declare return type of `CompletionStage`
@WrapsReturnValue
public class CompletionStageAsyncInvocation<T> implements AsyncInvocation<CompletionStage<T>> {
    @Override
    public CompletionStage<T> wrap(CompletionStage<T> original, Runnable completion) {
        return original.whenComplete((value, error) -> {
            completion.run();
        });
    }
}

All other async return types are similar, so I'm not gonna dive into those. That's the simple case.

For @Suspended AsyncResponse parameter type:

import jakarta.ws.rs.container.AsyncResponse;
import jakarta.ws.rs.container.Suspended;
import jakarta.ws.rs.container.TimeoutHandler;

// annotated `@WrapsArgumentValue` and first parameter of `wrap()` is of type `AsyncResponse`
// and annotated `@Suspended`, so we match all target methods that have exactly one parameter
// of type `AsyncResponse` that is annotated `@Suspended`
@WrapsArgumentValue
public class SuspendedAsyncInvocation implements AsyncInvocation<AsyncResponse> {
    @Override
    public AsyncResponse wrap(@Suspended AsyncResponse original, Runnable completion) {
        return new AsyncResponse() {
            @Override
            public boolean resume(Object response) {
                boolean resumed = original.resume(response);
                if (resumed) {
                    completion.run();
                }
                return resumed;
            }

            @Override
            public boolean resume(Throwable response) {
                boolean resumed = original.resume(response);
                if (resumed) {
                    completion.run();
                }
                return resumed;
            }

            @Override
            public boolean cancel() {
                boolean cancelled = original.cancel();
                if (cancelled) {
                    completion.run();
                }
                return cancelled;
            }

            @Override
            public boolean cancel(int retryAfter) {
                boolean cancelled = original.cancel(retryAfter);
                if (cancelled) {
                    completion.run();
                }
                return cancelled;
            }

            @Override
            public boolean cancel(Date retryAfter) {
                boolean cancelled = original.cancel(retryAfter);
                if (cancelled) {
                    completion.run();
                }
                return cancelled;
            }

            @Override
            public boolean isSuspended() {
                return original.isSuspended();
            }

            @Override
            public boolean isCancelled() {
                return original.isCancelled();
            }

            @Override
            public boolean isDone() {
                return original.isDone();
            }

            @Override
            public boolean setTimeout(long time, TimeUnit unit) {
                return original.setTimeout(time, unit);
            }

            @Override
            public void setTimeoutHandler(TimeoutHandler handler) {
                original.setTimeoutHandler(handler);
            }

            @Override
            public Collection<Class<?>> register(Class<?> callback) {
                return original.register(callback);
            }

            @Override
            public Map<Class<?>, Collection<Class<?>>> register(Class<?> callback, Class<?>... callbacks) {
                return original.register(callback, callbacks);
            }

            @Override
            public Collection<Class<?>> register(Object callback) {
                return original.register(callback);
            }

            @Override
            public Map<Class<?>, Collection<Class<?>>> register(Object callback, Object... callbacks) {
                return original.register(callback, callbacks);
            }
        };
    }
}

Out of curiosity, I also tried implementing AsyncInvocation for Kotlin suspend functions. I believe it should look like this:

@WrapsArgumentValue
class CoroutineAsyncInvocation<T> : AsyncInvocation<Continuation<T>> {
    override fun wrap(original: Continuation<T>, completion: Runnable): Continuation<T> {
        return object : Continuation<T> {
            override fun resumeWith(result: Result<T>) {
                completion.run()
                original.resumeWith(result)
            }

            override val context: CoroutineContext
                get() = original.context
        }
    }
}

I added matching annotations just because of JAX-RS, where AsyncResponse should always be annotated @Suspended, but I guess we could get rid of that for the purpose of invokers. I don't really mind.

WDYT?

[Azquelt]

In my opinion, we shouldn't require users to specify that they want method invocations to be asynchronous.

I disagree because I don't like having some magic that goes on behind the scenes and no way to verify that it's been applied correctly.

For instance, if I write an invoker for an async method that returns Uni, I don't want it to run correctly out of the box on Quarkus and blow up with a weird runtime exception on Liberty because some objects were destroyed too early. Instead I would want it to fail on startup on Liberty saying that there's no registered async handler applicable to the method.

If we do it this way, I think it's important that the user creating the invoker indicates that they expect it to be run asynchronously. It would however make sense to log a warning if the user has created a synchronous invoker for a method which looks asynchronous.

In other words, we should not define an API that all users have to use. We should define an SPI that containers should implement.

Depends what you mean by "containers". While the spec could require support for Jakarta async APIs and a CDI container could include support for others, any framework that defines a new async programming API would require a new implementation of the SPI, so we should allow users to provide one.

Thinking about it, it seems we can design a minimal API that doesn't give access to the entire invocation, just allows decorating the return value or the argument value.

I did consider this and I think it does cover all the cases I've looked at. The things that leaned me the other way were some hypothetical API we hadn't thought of and the inability to control when cleanup is done if the method throws an exception. These aren't particularly strong reasons so I'd be happy to do it this way. I think it's reasonable to clean up immediately if an exception is thrown as long as we require containers to allow cleanup to be called twice in case the async framework also calls it.

I don't think we should call it "wrap" - while wrapping the response or argument should be possible, it isn't always required. For example, the REST AsyncResponse example could just be:

@WrapsArgumentValue
public class SuspendedAsyncInvocation implements AsyncInvocation<AsyncResponse> {
    @Override
    public AsyncResponse wrap(@Suspended AsyncResponse original, Runnable completion) {
        original.register(new AsyncCallback(completion));
        return original;
    }

    public static class AsyncCompletion implements InvocationCallback<Object> {
        private Runnable completion;
        public AsyncCompletion(Runnable completion) {
            this.completion = completion;
        }

        public void completed(Object response) {
            completion.run();
        }

        public void failed(Throwable throwable) {
            completion.run();
        }
    }
}

Now the important part: how do we match the method return type (or parameter type) to the implementations of AsyncInvocation? By looking at the declaration of the first parameter of wrap(), especially on its type but also on its annotations.

I'm not sure we can do this for the annotations since we don't know whether or not we should ensure all the parameters of the annotation match. Qualifiers can annotate annotation members with @Nonbinding to indicate that they shouldn't be considered when matching annotations, but here we'd be dealing with annotations which were never intended to be compared like this.

I think we'd need another method which the the async handler class could optionally implement to allow it to examine the method or parameter and decide whether it handles that method or not.

[Ladicek]

I disagree because I don't like having some magic that goes on behind the scenes and no way to verify that it's been applied correctly.

For instance, if I write an invoker for an async method that returns Uni, I don't want it to run correctly out of the box on Quarkus and blow up with a weird runtime exception on Liberty because some objects were destroyed too early. Instead I would want it to fail on startup on Liberty saying that there's no registered async handler applicable to the method.

If we do it this way, I think it's important that the user creating the invoker indicates that they expect it to be run asynchronously. It would however make sense to log a warning if the user has created a synchronous invoker for a method which looks asynchronous.

This is a good argument. I've been thinking about this for the past few days and I currently see 3 possible designs:

1. The InvokerBuilder user says that the invocations on the built invoker are gonna be async and will provide an implementation of an API that allows saying when the async invocation is over and dependent objects may be destructed. This is basically your proposal.
2. The InvokerBuilder user says that the invocations on the built invoker are gonna be async, but will let the CDI container figure out the details. In other words, the API will expose something like withAsync(boolean), and there would have to be an SPI that the CDI container will automatically load and use to figure out when the async invocation is over and dependent objects may be destructed.
3. The InvokerBuilder is unchanged, there's just an SPI that the CDI container will automatically load and use to figure out which invocations are async and when the async invocation is over and dependent objects may be destructed. This is basically my proposal.

The upside of 1 and 2 is that they are explicit. The downside is that in both cases, there need to be two parties doing the same job of figuring out if the invocation is supposed to be async: the user of the InvokerBuilder API (to know if they should call the withAsyncHandler()/withAsync() API), and the implementor of the API/SPI (to validate that what the user asks for is actually correct).

Case 3 is the exact opposite: there's implicit behavior and there's zero work on the InvokerBuilder user side.

I don't think it makes any sense to call async methods synchronously. I'm obviously biased :-), but this makes me prefer case 3.

(Aside: I do think that if an async method throws synchronously, cleanup should be synchronous as well, but that should be an exceptional case.)

I think case 3 could be made a little safer by the CDI specification recommending that implementations detect building invokers for methods that look async but don't have matching SPI implementation, and failing the deployment in that case. Given that there's a fairly small number of additional async types out there in the wild (RxJava 1/2/3, Reactor, Mutiny, and that's pretty much it? I think we can ignore Kotlin suspend functions here), this should be doable. And since all the additional async types are used as method return types, the check would be just a lookup in a constant set of class names.

In other words, we should not define an API that all users have to use. We should define an SPI that containers should implement.

Depends what you mean by "containers". While the spec could require support for Jakarta async APIs and a CDI container could include support for others, any framework that defines a new async programming API would require a new implementation of the SPI, so we should allow users to provide one.

Right. I think CDI should require support for CompletionStage (and most likely also for CompletableFuture). JAX-RS should demand support for @Suspended AsyncResponse. All other async types are implementation defined.

Thinking about it, it seems we can design a minimal API that doesn't give access to the entire invocation, just allows decorating the return value or the argument value.

I did consider this and I think it does cover all the cases I've looked at. The things that leaned me the other way were some hypothetical API we hadn't thought of and the inability to control when cleanup is done if the method throws an exception. These aren't particularly strong reasons so I'd be happy to do it this way. I think it's reasonable to clean up immediately if an exception is thrown as long as we require containers to allow cleanup to be called twice in case the async framework also calls it.

I don't think we should call it "wrap" - while wrapping the response or argument should be possible, it isn't always required.

Agree, there's better naming for sure.

For example, the REST AsyncResponse example could just be:

@WrapsArgumentValue
public class SuspendedAsyncInvocation implements AsyncInvocation<AsyncResponse> {
    @Override
    public AsyncResponse wrap(@Suspended AsyncResponse original, Runnable completion) {
        original.register(new AsyncCallback(completion));
        return original;
    }

    public static class AsyncCompletion implements InvocationCallback<Object> {
        private Runnable completion;
        public AsyncCompletion(Runnable completion) {
            this.completion = completion;
        }

        public void completed(Object response) {
            completion.run();
        }

        public void failed(Throwable throwable) {
            completion.run();
        }
    }
}

Great point, and nice simplification. Thanks!

Now the important part: how do we match the method return type (or parameter type) to the implementations of AsyncInvocation? By looking at the declaration of the first parameter of wrap(), especially on its type but also on its annotations.

I'm not sure we can do this for the annotations since we don't know whether or not we should ensure all the parameters of the annotation match. Qualifiers can annotate annotation members with @Nonbinding to indicate that they shouldn't be considered when matching annotations, but here we'd be dealing with annotations which were never intended to be compared like this.

Yeah, since annotation matching would AFAICT only apply to @Suspended AsyncResponse, I think we can just get rid of it and only match on types. I think that would be just fine. In that case, we could match just on the type argument of the superinterface type, but that's a minor detail.

[Azquelt]

The upside of 1 and 2 is that they are explicit. The downside is that in both cases, there need to be two parties doing the same job of figuring out if the invocation is supposed to be async: the user of the InvokerBuilder API (to know if they should call the withAsyncHandler()/withAsync() API), and the implementor of the API/SPI (to validate that what the user asks for is actually correct).

This is true, but I think the user is the only one who actually knows whether the invocation is supposed to be async. Though the container can be reasonably sure it is async if it has a known async return type, it can't know about things it doesn't know about.

Remember here that the InvokerBuilder user is likely a framework author, and not an application developer. They presumably already need to know whether the call is asynchronous since they are going to handle the result differently in each case, so I don't think that having them tell us that is particularly arduous.

I think the portability aspect is also more important because this work is aimed at framework authors and portability is more important for a framework than it is for an application. If you want to write a portable framework and you want to support an asynchronous representation other than CompletionStage, you're going to have to include an AsyncInvocation class (or depend on a library which provides one) since you can't rely on the container including one. You will realise this very quickly if we require frameworks to indicate that the method is asynchronous, warn if we see something that looks like it ought to be asynchronous, and validate that an appropriate AsyncInvocation class is available, whereas it would be easy to miss if we didn't do this.

[Ladicek]

I was thinking about different shapes of the API, but everything I could figure out was a variant of the 3 possible options I outlined in my previous comment.

Assuming it doesn't really matter whether the InvokerBuilder has withAsyncHandler(AsyncHandler) or withAsync(boolean), the difference boils down to: is it worse to forget to call the withAsync API, or forget to add the implementation of the automatically-used SPI? Preferrably, both should be hard errors, which leads me to the following.

If we agree that

1. forgetting to call the withAsync API is a deployment problem for all async types we know about, and
2. calling the withAsync API is a deployment problem if we cannot recognize the async type of the method,

then the withAsync API is just an explicit designation that confirms what the container can already figure out on its own. Its value is only in that it allows detecting the 2 issues mentioned above. That itself is good enough.

So here's a refined proposal that is in principle close to yours, but the API itself is close to mine :-)

The InvokerBuilder gains a method withAsync() (or withAsync(boolean), doesn't matter) which marks the built invoker as asynchronous. Asynchronous invoker destroys dependent instances after the asynchronous action completes, not after the method returns/throws.

Asynchronous invokers may only exist for asynchronous methods. If a method is asynchronous (see below), the invoker must be asynchronous, otherwise deployment problem occurs. If a method isn't asynchronous (see below), the invoker must not be asynchronous, otherwise deployment problem occurs.

A method is asynchronous if an implementation of AsyncInvocation (as described in my previous post) is discovered (via ServiceLoader) for either the method's return type, or for one of the method's parameter types.

CDI container must provide an implementation of AsyncInvocation for:

• java.util.concurrent.CompletionStage
• java.util.concurrent.CompletableFuture
• java.util.concurrent.Flow.Publisher

(In my previous comments, I ignored reactive streams, but I believe we should include them as well.)

CDI containers may (but are not required to) provide implementations of AsyncInvocation for other async types.

JAX-RS will likely demand an implementation of AsyncInvocation for jakarta.ws.rs.container.AsyncResponse, although it's probably useless outside of JAX-RS itself.

I'm actually thinking: I don't think it makes sense to invoke asynchronous methods synchronously, but if we wanted to have that ability, we could demand that if a method is asynchronous, withAsync(boolean) is always called, otherwise deployment problem occurs. An asynchronous method may then be invoked synchronously, if the call to withAsync(boolean) passed the false argument. Explicit over implicit. I kinda like this.

[Azquelt]

So here's a refined proposal that is in principle close to yours, but the API itself is close to mine :-)
...

I think this makes sense, I'll work on writing this up.

JAX-RS will likely demand an implementation of AsyncInvocation for jakarta.ws.rs.container.AsyncResponse, although it's probably useless outside of JAX-RS itself.

I'm not sure whether this will be necessary. The use of invokers will likely be internal to the JAX-RS implementation. They could require it anyway - the only advantage I can see would be consistency for users calling methods with AsyncResponse parameters - you wouldn't end up with them being asynchronous in some implementations but not in others.

[Ladicek]

FTR, I started working on spec text here: https://github.com/jakartaee/cdi/compare/main...Ladicek:cdi-spec:async-invokers?expand=1

[Azquelt]

Ah! Thank you