allow asynchronous fault tolerance strategies without offloading to an extra thread

[Ladicek]

So we have asynchronous fault tolerance strategies for CompletionStage-returning methods, but those only apply if the method is also @Asynchronous. If the method is not annotated @Asynchronous, then all the fault tolerance strategies treat the method as synchronous and only apply around the method call.

If I have a truly asynchronous method, I don't want to add @Asynchronous, because that leads to an explicit move to an extra thread. (There can be several reasons why I don't want that. For example, the method itself can already spawn an extra thread. Or the method can execute on an event loop. Etc.)

This is not a made-up scenario -- I found about this issue when trying to use MP RestClient with MP Fault Tolerance! In MP RestClient, if the method returns CompletionStage, that itself means that the RestClient implementation needs to run the method asynchronously. Adding @Asynchronous means that Fault Tolerance will also run it asynchronously. That's a waste of resources.

In other words, the @Asynchronous annotation has a very narrow meaning -- basically, it always means offloading to an extra thread. That is very limiting. For example, in an event loop world, everything is asynchronous yet running on a single thread (or a small thread pool).

I believe we need to add an ability to let CompletionStage-returning methods be treated as asynchronous without offloading to an extra thread. I can see several ways of achieving that:

• If a CompletionStage-returning method isn't annotated @Asynchronous, we still apply all the fault tolerance strategies asynchronously. Here, the @Asynchronous annotation basically retains the narrow meaning of "force offloading to an extra thread".
• Variant of the above option: rename @Asynchronous to @ThreadOffload, so that it's clear what the annotation does. We'd still treat the return type as the primary indicator of whether the method is asynchronous or not.
• Modify the @Asynchronous annotation to be able to signalize whether offloading to an extra thread is desired. For example, @Asynchronous(threadOffload = true) to offload to an extra thread, and @Asynchronous(threadOffload = false) for staying on the original thread. Here, the @Asynchronous annotation becomes more general. We'd probably default threadOffload to true to stay compatible, and trust user that they know they need to set threadOffload = false.
• Perhaps more, can't think of anything right now.

[Azquelt]

This was a deliberate choice because retries will have to run on a different thread and we didn't want to have the first request run on the same thread, but then have retries run on a different thread in case that lead to situations where the first attempt would always work but retries would always fail because of some difference when running asynchronously.

Not saying it's something that we can't revisit, but I think that was the original rationale.

[Ladicek]

Not sure retries always have to run on a different thread. Just need to make sure that the delay between retries isn't Thread.sleep :-)

[kenfinnigan]

+1 to renaming @Asynchronous to tie it with separate thread execution.

It shouldn't be necessary to use @Asynchronous if you're returning a CompletionStage, or any other asynchronous type.

I don't believe there's a need for retries to always be on a separate thread, there should be ways to make it runnable on the same thread, or a mixture of the two. Where the retries are executed should be up to a runtime to decide what is best for it, based on its threading model.

[Emily-Jiang]

@Ladicek We discussed this before as mentioned by @Azquelt ! The fact is that the method return type does not force the method itself running asynchronously.

The main usage of @Asynchronous is to be with @Bulkhead, which means it is the thread pool isolation. I don't really get the naming confusion. @Asynchronous means the method will run asynchronously. As for which thread the Fault Tolerance needs to be run, it is an implementation detail.

We discussed whether to update @Timeout with @Asynchrous or not to behave the same. We discussed and agreed to @Asynchronous means asynchronous. Without it, it means the method will not run asynchronous and it does as it is told.

[Ladicek]

@Emily-Jiang Unfortunately, no. @Asynchronous as it is currently specified doesn't mean that the method will run asynchronously. It means the method will always be executed on an extra thread. If the method is already executed asynchronously (such as an MP RestClient method that returns CompletionStage), this leads to waste of resources.

[Ladicek]

However, it's a good point that presence or absence of @Asynchronous modifies the @Bulkhead behavior. That, IMHO, is wrong. The @Bulkhead annotation should itself configure whether semaphore isolation or "threadpool" isolation should be used. We can easily do that by changing the default value of waitingTaskQueue to 0 (where 0 means semaphore isolation), and whenever user configures it to > 0, "threadpool" isolation (which is in fact just an additional queue) is used. I'll create another issue for that, as that's unrelated to this topic.

EDIT: filed #535 for the @Bulkhead thing. We can discuss @Bulkhead with or without @Asynchronous there; I'd like this issue to stay on topic.

[kenfinnigan]

@Emily-Jiang Surely it's more practical to define that a return type of CompletionStage, or equivalent, requires asynchronous execution like MP REST Client.

But what exactly that asynchronous execution means is up to the implementation. It could be a separate thread, or it could be asynchronously on the same thread.

The main benefit is that the behavior is implied by the return type as opposed to needing to be specified.

For those familiar with CompletionStage and other asynchronous types, it would be an anti-pattern, of sorts, to need to explicitly indicate @Asynchronous in addition to the return type.

Having consistency across specifications that an asynchronous return type means the method is executed asynchronously is very important

[Emily-Jiang]

@Ladicek @kenfinnigan I thought a bit more based on your comments. Maybe we can take this release to make behaviour changes by removing @Asynchronous and determine whether it is async or not based on the return type. Let's discuss more in our next week hangout and visit all of the combinations with other FT annotations.

[Azquelt]

I think MP Rest Client is a somewhat different case. With MP Rest Client, the user calls a method provided by the implementation. In contrast, with MP Fault Tolerance, both the caller and the method body are provided by the user.

It seems odd to me that I could write a method returning a CompletionStage and use it, but then if I add @Retry it suddenly starts running asynchronously.

[Ladicek]

The main idea here is that CompletionStage-returning methods should already be running asynchronously. The @Asynchronous annotation is an easy way how to let a synchronous method run asynchronously by offloading to another thread, which is something I'd certainly like to retain, but should be named accordingly.

[Ladicek]

Yesterday, I realized there's probably a better way to explain what I mean:

The current @Asynchronous annotation means "this method is synchronous and I want you to make it asynchronous". There's no way to signal "this method is already asynchronous and I want you to treat it as such".

There are several options:

• keep an annotation for "make asynchronous" and use return type for detecting "is asynchronous"
• use one annotation for "make asynchronous" and another annotation for "is asynchronous"
• use one annotation for all asynchronous methods, with an attribute that decides whether to also "make asynchronous"
• perhaps more?