eclipse / microprofile-fault-tolerance

microprofile fault tolerance
Apache License 2.0
131 stars 64 forks source link

The priority for the interceptors of FT #186

Closed Emily-Jiang closed 5 years ago

Emily-Jiang commented 7 years ago

FT introduced a number of interceptor bindings. We should spec that the interceptors should be the first interceptors to be invoked, which means having the lowest priority. Thoughts?

Pandrex247 commented 7 years ago

I'd say the Asynchronous interceptor should be invoked either first or last, as to me it makes sense for the other interceptors (bulkhead etc.) to happen on the same thread - either the invoking thread, or the asynchronous one. My preference would be for first, as then the exception handling for any fault tolerance exceptions is pushed to the user as part of their Future.get() (in which they're already having to do some exception handling), rather than failing fast beforehand. It does briefly "waste" a thread resource though if one of the other interceptors throws an exception before the actual annotated method executes so... ¯_(ツ)_/¯

As I've asked for clarification on in the Gitter chat, the TCK seems to be assuming that the Asynchronous annotation is actually happening last, as a couple of tests just swallow any exceptions that come from Future.get(), even if they're expected exceptions. So that may be a factor.

Apart from that, the ordering that jumps out at me is: Fallback > Retry > Bulkhead > CircuitBreaker > Timeout. My reasoning being that Fallback needs to be the first so that it can catch any exceptions propagated up by the others, followed by Retry, for exactly the same reason. You'd then check to see if there's a free semaphore/token for the thread, before checking the circuit state and attempting to execute. I'd say Bulkhead and CircuitBreaker are fairly interchangeable though. Timeout should be last, as to me it doesn't make sense to start the Timeout until you're just about to execute the actual method logic, plus it's the interceptor that's going to be throwing the timeout exception which needs to propagate up through each of the others so they can do their failure action (e.g. open the circuit).

tevans78 commented 6 years ago

One of the reasons that ordering problems arise is because the annotations are not completely independent and it does not always make sense for one to be "before" the other. @Asynchronous and @Bulkhead is the most obvious example since it is called out by the spec...

When @Bulkhead is used with @Asynchronous, the thread pool isolation approach will be used. If @Bulkhead is used without @Asynchronous, the semaphore isolation approach will be used.

i.e. @Asynchronous changes the behaviour of @Bulkhead.

OndroMih commented 5 years ago

This is partially solved by enforcing that there's a single interceptor providing all the functionality, with PR #338. The spec still misses detailed description how interceptors are ordered and how they all work together. This information is scattered across specification of individual annotation and is even incomplete. For example, I still didn't find any clarification of whether @Asynchronous functionality is applied before or after other annotations.

We already had some discussion about interceptor ordering here: https://github.com/eclipse/microprofile-fault-tolerance/issues/291

There are several issues related to asynchronous execution and clarification of the order of interceptors. I'm loosing track of them all. We should probably merge some of them or mark them as related (e.g. with a tag). I at least tried to link some of them together.

Azquelt commented 5 years ago

I'm slightly confused, you seem to say that there's a single interceptor, but then go on to say that we don't define how the interceptors are ordered. I'll assume you're asking for the order that the logic required by each annotation should be applied (even if it's applied within one interceptor).

Currently, the spec doesn't define an ordering, but it should define the interactions between annotations. If there's something missing, do let us know what it is.

In the case of @Asynchronous, we don't define that "Asynchronous happens first" but we do define that the method immediately returns a Future or CompletionStage, which precludes doing any waiting operation on the calling thread.

This means that on the calling thread an implementor could:

However, they mustn't:

In addition, most of the difficulties in specifying interactions between interceptors occur when you have @Asynchronous and two or more other annotations. E.g.

If @Timeout is used with @Retry, a TimeoutException may trigger a retry, depending on the values of retryOn and abortOn of the @Retry annotation. The timeout is restarted for each retry. If @Asynchronous is also used and the retry is the result of a TimeoutException, the retry starts after any delay period, even if the original attempt is still running.

These sorts of rules can't be defined by declaring an order.

Emily-Jiang commented 5 years ago

dup of #313

Emily-Jiang commented 5 years ago

please reopen if you think otherwise.

OndroMih commented 5 years ago

I agree with merging with #313, it's better to focus on a single issue.