specification of asynchronous invokers

[Ladicek]

Fixes #859

[Ladicek]

This is an initial proposal. I assume you will have some questions about certain aspects of the API.

Here are my questions that I don't have a full answer to at the moment:

1. Currently, we specify that withAsync() must not be called for non-async methods. We could allow calling withAsync(false); should we?
2. Currently, we specify that "it is recommended that completion is signalled before it is propagated to the caller of Invoker.invoke()"; should we make that mandatory? I think it's straightforward to signal completion before propagating it to the caller for all types except JAX-RS AsyncResponse. I don't know what happens if I register an InvocationCallback on the AsyncResponse, so I went with a recommendation for now.
3. Similarly to the previous question, I'm also not sure about async methods throwing synchronously. Again, it seems straightforward for all types except JAX-RS AsyncResponse.

[Azquelt]

Provider constructor?

[Ladicek]

This is specified in java.util.ServiceLoader.

I'm actually not sure if we need this provision, but if we allow provider methods, the service provider doesn't have to implement AsyncHandler and figuring out the async type would be harder. I'm not sure if anyone actually uses provider methods; the other parts of the CDI spec ignore them completely...

[Ladicek]

One other question I don't currently have an answer for: what if there's multiple async handlers for the same async type?

[Azquelt]

1. Currently, we specify that `withAsync()` must not be called for non-async methods. We could allow calling `withAsync(false)`; should we?

I think we should allow withAsync(false) to state that the method should be treated non-asynchronously.

2. Currently, we specify that "it is recommended that completion is signalled before it is propagated to the caller of `Invoker.invoke()`"; should we make that _mandatory_? I _think_ it's straightforward to signal completion before propagating it to the caller for all types except JAX-RS `AsyncResponse`. I don't know what happens if I register an `InvocationCallback` on the `AsyncResponse`, so I went with a recommendation for now.

I think a recommendation makes sense. If you have a listener model, I don't think you can guarantee that.

3. Similarly to the previous question, I'm also not sure about async methods throwing synchronously. Again, it seems straightforward for all types except JAX-RS `AsyncResponse`.

It's straightforward when the return type represents the async task. If it's an argument, then I don't think there's an obvious answer as to whether it'll call the completion task or not.

I would suggest that we say that cleanup will happen immediately if the invocation throws an exception, and that the container must tolerate the completion being run as well.

[Ladicek]

3. Similarly to the previous question, I'm also not sure about async methods throwing synchronously. Again, it seems straightforward for all types except JAX-RS `AsyncResponse`.

It's straightforward when the return type represents the async task. If it's an argument, then I don't think there's an obvious answer as to whether it'll call the completion task or not.

I would suggest that we say that cleanup will happen immediately if the invocation throws an exception, and that the container must tolerate the completion being run as well.

Hmm, that makes sense. So something like the following? The first sentence is copied, the second is new.

If an asynchronous target method throws synchronously, instances of @Dependent looked up beans are destroyed before invoke() rethrows the exception.
In this case, the async handler is permitted to call completion.run(), but the CDI container must ignore that.

[Azquelt]

One other question I don't currently have an answer for: what if there's multiple async handler for the same async type?

I'm a bit concerned about this too since I think it's likely that multiple libraries will provide an async handler for common async types where support from the container is not required.

We could say that any of them may be called, which is a bit unpredictable, but in theory fine as long as all the deployed async handlers are correct.

We could say that it's a deployment error, though that might be difficult for the user to resolve if the handlers are provided by libraries.

I'm also a bit concerned about the scope of an async handler in a multi-module scenario. If I deploy an async handler in a module, is it available for use by the whole application, or only invocations of beans in modules which have visibility of the async handler module.

[Azquelt]

Hmm, that makes sense. So something like the following? The first sentence is copied, the second is new.

If an asynchronous target method throws synchronously, instances of @Dependent looked up beans are destroyed before invoke() rethrows the exception.
In this case, the async handler is permitted to call completion.run(), but the CDI container must ignore that.

That looks good to me. Maybe tweak it slightly:

In this case, the async handler is still permitted to call completion.run(), but the CDI container must ignore it.

[Ladicek]

I was thinking about the issue with multiple async handlers existing for the same async type. As we discussed on the CDI call today, we could add withAsyncHandler(Class<? extends AsyncHandler), but:

1. this would make withAsync(boolean) quite useless (not necessarily bad),
2. if done naively, this would prevent detecting the situation when an async target method exists but withAsync*() isn't called for it.

To avoid issue 2, we'd have to still require that async handlers are service providers of the AsyncHandler interface, so that the CDI container can find all of them and hence be able to figure out what methods are async.

There are some usability issues with this, but they are minor compared to the issue we'd solve. I'll think more about it.

[Ladicek]

I've adjusted this PR to what I described in my last comment. Async handlers must still be service providers, and they are selected explicitly using withAsync(). This allows cross-checking during deployment.

I'm not super happy about this proposal, but it isn't bad either.

[Ladicek]

As I mentioned on the CDI call earlier this week, I wanted to list all possible issues the current design is supposed to prevent. Here goes.

1. If the user forgets to call withAsync() but the async handler is registered, we can detect that.
2. If the user calls withAsync() but forgets to register the async handler, we can detect that.
3. I don't think we can detect a situation when the user forgets to call withAsync() and forgets to register the async handler. This is not a problem in case of the built-in async types (CompletionStage, CompletableFuture, Flow.Publisher) and shouldn't be a big deal in case of platforms that include async handlers for their common async types. It is problematic in other situations.

If we removed the need to register async handlers (via the service loader mechanism), we would rely on users to never forget to call withAsync(). Alternatively, we could treat all async handlers passed to withAsync() as registered and check all built invokers against those, but that doesn't prevent the problem, it just makes it less likely.

If we removed the [need to call the] withAsync() method, we would rely on users always registering async handlers. This is not a terribly huge price to pay, but there would be a bigger problem: which async handler should we choose if multiple are registered for the same async type?

All in all, I think there's no better alternative to the current design.

[Ladicek]

I slightly adjusted the API (renamed AsyncHandler.CompletionStage to AsyncHandler.ForCompletionStage and similar) and I believe this is ready for final review and merging.

[Azquelt]

Do we want to restrict calling withAsync(null) for non-asynchronous methods?

I would change this to "must not be called with a non-null value when the target method is not asynchronous"

[Ladicek]

I would prefer to keep it, because it's nicely symmetric: if the target method is async, this method must be called, and if the target method is not async, this method may not be called.

[Azquelt]

I get that, but it does mean that if another library is added which adds an async handler for your return type, you need to call withAsync(null) if that library is present and not call it if it isn't present.

[Ladicek]

I think that situation is weird enough that users want to know about it instantly. (Also it seems quite unlikely. There's not that many async types out there.)

[Azquelt]

It does seem unlikely, but I really dislike the idea of having to call the method if an unrelated library is present and not call it if the library is not present and having no way of writing the code that works in both situations.

I guess you could argue that the developer can use reflection to detect whether the other library is present, but that seems more nasty and error prone.

[Ladicek]

I would say if you found yourself in such situation, you want know about it and you need to think about it. Using a null handler may be a valid solution, but I'd argue that more often than not, there will be other things you want to do.

[Azquelt]

Suggested change

	* @param asyncHandler the {@link AsyncHandler} to use; may be {@code null}
	* @param asyncHandler the {@link AsyncHandler} to use, or {@code null} to indicate that the invoker is synchronous

[Azquelt]

Suggested change

	If the target method is not asynchronous, the `withAsync()` method on `InvokerBuilder` must not be called, otherwise deployment problem occurs.
	If the target method is not asynchronous, the `withAsync()` method on `InvokerBuilder` must not be called with a non-`null` value, otherwise deployment problem occurs.

If we make the corresponding change in the Javadoc.

[Ladicek]

Need to think about (and likely mention in the spec) classloading restrictions:

• the invoker builder must obtain a class of the async handler, which means that if the async handler class is not accessible to the registration place, the invoker builder invocation would fail
• building invokers must check if the method is async or not, which requires looking up all registered async handlers, which probably must be scoped to the bean manager

[Ladicek]

what if there's multiple async handlers for the same async type?

I was thinking about this yesterday and I think we forgot to consider one other option: this could just be a deployment problem. The user can overcome this by explicitly configuring the class of AsyncHandler they want to use for a given async type. This configuration could even be used to override the built-in AsyncHandlers for JDK-provided async types. Since I believe this situation will actually be rare in practice, I'd be fine with mandating CDI containers to provide an implementation-defined means of this configuration, but I could be persuaded to actually have a specification for this.

System properties come to mind, but those are JVM-global, so probably not a good fit for multi-deployment application servers. (They would be fine for CDI SE though.) Using beans.xml is wrong, because that includes configuration for the bean archive, not for the whole deployment. (It could be doable, if we merged those configurations and verified for duplicates, but it still feels wrong.) I don't immediately see any other option, but let's see what others think about the idea first :-)

I think this would actually allow simplifying the API to the bare minimum (just the AsyncHandler interface and the 2 annotations). It wouldn't allow forcing synchronous destruction for asynchronous methods, but that has always felt dubious to me, so I don't see that as problematic. It wouldn't require the InvokerBuilder.withAsyncHandler() method at all, because matching would be automatic and deterministic.

One possible problematic scenario would remain: user has an asynchronous method, but doesn't include an AsyncHandler for the async type, so dependent objects injected for the method are destroyed synchronously. But this is a problem we already have, even with the current specification, and I don't see a way to avoid it. The only possible thing we can do in the specification is to encourage CDI implementations to check for common async types (RxJava, Mutiny, Reactor, that's probably it?) and log a warning if an AsyncHandler is not registered.

Ideas?

[Ladicek]

Rebased and pushed another commit (so it's easy to revert) that alters the specification per my previous comment.

[Azquelt]

I was thinking about this yesterday and I think we forgot to consider one other option: this could just be a deployment problem.

It could be, my initial concern is that it makes it harder to write portable libraries, but based on the discussion today, I'm thinking again about whether that might be unfounded, and maybe I don't hate it so much 😉

Here's the scenario I was imagining:

I write a framework built on CDI which uses invokers (I'm thinking of something similar to the quarkus mcp server extension). I want my framework to support mutiny, so I allow methods to return Uni. For this to work, I need an AsyncHandler implementation for Uni.

I was imagining that my framework would:

• have a dependency on mutiny
• provide and use an AsyncHandler for Uni (since it can't rely on CDI to provide one)
• as a user, I add a dependency on the framework maven artifact and everything I need is packaged in my application

However, based on the discussion today, maybe it would actually be built like this:

• have a compile-only dependency on mutiny
• rely on the platform or someone else to provide both mutiny and an AsyncHandler for mutiny
  • if deployed on Quarkus, mutiny and the handler are already included
    • as a user, I still just need a dependency on the framework maven artifact
  • if deployed on Open Liberty, neither is provided and the user would need to include both mutiny and the handler in their application
    • as a user, I need a dependency on the framework maven artifact, a dependency on mutiny, and a dependency on an AsyncHandler for mutiny

In the second case, staying portable is easier, but only because you're effectively saying the user has to provide your dependencies manually. However, it does also mean that the user is the one with the ability to fix the deployment problem.

---

We do lose the ability for anyone to detect if the framework is deployed with the AsyncHandler. This is a bit of a concern, since frankly I think users are more likely to screw this up than framework authors.

In general I think the framework has an interest in detecting this situation because it's them that will have to deal with users raising issues where the root cause is that the user didn't provide an AsyncHandler. Possible fixes would be:

• reintroduce a way for the framework to indicate "this method is async, report an error if the AsyncHandler is missing"
  • if they don't call it, nothing bad actually happens, we just don't know to validate that there is an AsyncHandler
• add a way for the framework to query for an async handler, so that it can report an error itself

I do agree that keeping the API simpler is preferable. After weighing it up I don't mind losing the ability to force synchronous destruction for asynchronous methods and making the validation a little less strict if it keeps things simpler.

[Ladicek]

Sorry for ignoring this topic for so long. I think your objections make sense and I'd be fine with removing the 2nd commit. But it also feels like it should be straightforward to add the ability for CDI-based frameworks to indicate that they require presence of AsyncHandlers for given types. During AfterDeploymentValidation or @Validation seems best. There's multiple ways we could do it, but a validation method ensureAsyncHandlerPresent(Class<?>) should be enough. For portable extensions, this could be simply added to AfterDeploymentValidation; I'm not exactly sure about build compatible extensions, we don't have a decent place for this method already, but adding one shouldn't be too hard.