smallrye / smallrye-fault-tolerance

SmallRye implementation of MicroProfile Fault Tolerance: bulkheads, circuit breakers, fallbacks, rate limits, retries, timeouts, and more
Apache License 2.0
90 stars 37 forks source link

Fault tolerance annotations on fallback methods #442

Open spyrkob opened 3 years ago

spyrkob commented 3 years ago

Fault tolerance annotations are silently ignored on fallback methods. For example:

   @Fallback(fallbackMethod = "legacyFindBook")
   public Book findBookInOnlineStore(String title) {
      ...
   }

   @Bulkhead(value = 1)
   public Book legacyFindBook(String title) {
      ...
   }

The @Bulkhead on legacyFindBook will be ignored and if there are concurrent calls to findBookInOnlineStore multiple threads will be able to execute the fallback method.

If this is planned maybe it would be useful to print a warning in such cases?

Ladicek commented 3 years ago

They are not "silently ignored", because there's nothing in the MicroProfile Fault Tolerance specification saying that this should work. More specifically, the fault tolerance annotations are interceptor binding annotations, and interceptors are only applied for business method invocations (see https://docs.jboss.org/cdi/spec/2.0/cdi-spec.html#biz_method). The MP FT specification would have to explicitly say that invocations of fallback methods are business method invocations -- which it doesn't.

I guess the reason is simple: @Fallback is a mechanism for providing, well, fallback values.

There seems to be a growing tendency of trying to coerce @Fallback into a generic exception handling mechanism. That's not its purpose, and I'm very reluctant to add support for anything that would move in that direction.

What you're attempting here falls into that bucket too.

Here's a simple fix:

// you might want to still have a `@Fallback` here, because
// `legacyFindBook` will throw in case of concurrent access
public Book findBookInOnlineStore(String title) {
    try {
        ...
    } catch (Exception e) {
        return legacyFindBook(title);
    }
}

@Bulkhead(value = 1)
public Book legacyFindBook(String title) {
    ...
}

This will work fine in environments that support self-interception, such as Quarkus. In environments that don't, such as WildFly, you'd have to use self-injection.

Printing a warning for fallback methods that have FT annotations is certainly an option. I guess fallback methods shouldn't be called from anywhere else, so such warning is probably safe.