quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.87k stars 2.71k forks source link

Quarkus Cache: Improved Fallback Fault Tolerance #34779

Open AndreasPetersen opened 1 year ago

AndreasPetersen commented 1 year ago

Description

This is probably mostly relevent for the Redis backend for Quarkus Cache.

Consider the following:

@Path("/hello")
public class GreetingResource {
    @GET
    public String hello() {
        return expensive();
    }

    @CacheResult(cacheName = "myCache")
    @Fallback(fallbackMethod = "expensiveFallback")
    String expensive() {
        return expensiveFallback();
    }

    String expensiveFallback() {
        return "Hello from RESTEasy Reactive";
    }
}

using Quarkus 3.2.0, quarkus-redis-cache and quarkus-smallrye-fault-tolerance.

First I make a call to hello() thus populating the cache. When I then close Redis, to simulate a failure to get values from the cache, and call hello(), I get the following exception:

13:30:04 ERROR [io.qu.ve.ht.ru.QuarkusErrorHandler] (executor-thread-1) HTTP Request to /hello failed, error id: dc914983-726f-4830-8b4e-20ccba36894e-1: java.util.concurrent.CompletionException: io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: localhost/127.0.0.1:6379
        at io.smallrye.mutiny.operators.uni.UniBlockingAwait.await(UniBlockingAwait.java:79)
        at io.smallrye.mutiny.groups.UniAwait.atMost(UniAwait.java:65)
        at io.smallrye.mutiny.groups.UniAwait.indefinitely(UniAwait.java:46)
        at io.quarkus.cache.runtime.CacheResultInterceptor.intercept(CacheResultInterceptor.java:104)
        at io.quarkus.cache.runtime.CacheResultInterceptor_Bean.intercept(Unknown Source)
        at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
        at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
        at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
        at org.acme.GreetingResource_Subclass.expensive(Unknown Source)
        at org.acme.GreetingResource.hello(GreetingResource.java:15)
        at org.acme.GreetingResource$quarkusrestinvoker$hello_e747664148511e1e5212d3e0f4b40d45c56ab8a1.invoke(Unknown Source)
        at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
        at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
        at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:145)
        at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:576)
        at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
        at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)
        at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:29)
        at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:29)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: localhost/127.0.0.1:6379
Caused by: java.net.ConnectException: Connection refused
        at java.base/sun.nio.ch.Net.pollConnect(Native Method)
        at java.base/sun.nio.ch.Net.pollConnectNow(Net.java:672)
        at java.base/sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:946)
        at io.netty.channel.socket.nio.NioSocketChannel.doFinishConnect(NioSocketChannel.java:337)
        at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.finishConnect(AbstractNioChannel.java:334)
        at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:776)
        at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:724)
        at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:650)
        at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:562)
        at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
        at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:833)

The @Fallback annotation does not work in combination with @CacheResult. Instead, I have to wrap my cached method in order to achieve fallback fault tolerance:

@Path("/hello")
public class GreetingResource {
    @GET
    public String hello() {
        return wrappedExpensive();
    }

    @Fallback(fallbackMethod = "expensiveFallback")
    String wrappedExpensive() {
        return expensive();
    }

    @CacheResult(cacheName = "myCache")
    String expensive() {
        return expensiveFallback();
    }

    String expensiveFallback() {
        return "Hello from RESTEasy Reactive";
    }
}

I see a number of issues:

  1. @Fallback does not work as expected in combination with @CacheResult
  2. The exception thrown by the cached method expensive() when Redis is closed is too generic: A CompletionException with a root cause of ConnectException. If the code inside the cached method could also throw a ConnectException, then it is not possible to define a @Fallback that only applies for the case where the cache itself fails.
  3. The two examples given include a lot of boilerplate that I have to add to every single cached method, in order to have fallback fault tolerance. For my case, I would like the code in the cached method to be executed, in case the cache fails.

I would like to request:

  1. @Fallback should work in combination with @CacheResult
  2. A specific exception thrown when retrieving from the cache failed, for example a CacheFailure exception
  3. A convenient way of adding fallback fault tolerance to cached method. Maybe this could be done by simply annotating the cached method with @Fallback and not providing a fallbackMethod? I'm not sure how this would follow the Fault Tolerance spec. Alternatively, add an option to @CacheResult(fallback=true) that would execute the code inside the cached method, in case of a cache retrieval failure.

Implementation ideas

No response

quarkus-bot[bot] commented 1 year ago

/cc @Ladicek (fault-tolerance), @gwenneg (cache)

Ladicek commented 1 year ago

The priority of the fault tolerance interceptor (which is what handles @Fallback) is Interceptor.Priority.PLATFORM_AFTER + 10 by specification.

The priority of the @CacheResult interceptor is Interceptor.Priority.PLATFORM_BEFORE + 2.

That is, the caching interceptor runs first. If the caching interceptor itself does some network interactions (talking to Redis in this case), that cannot be guarded by fault tolerance. It is only when the caching interceptor finds that it doesn't have a cached value and attempts to compute it that fault tolerance may come to play. The caching interceptor in such case attempts to call the method (through InvocationContext.proceed()), and that call goes through the fault tolerance interceptor. When that call returns, either with the return value of the method or with the fallback value, that result will be cached.

That might not be "as expected", but it is certainly "as designed".

Splitting the method into two, as you found, is a good way to make sure the interceptors are applied in the order you desire.

AndreasPetersen commented 1 year ago

If it works as designed, can we make a better design? I have to split the code into three methods, in order to achieve the desired fallback. This is a lot of boilerplate to add to all our cached methods.

Ladicek commented 1 year ago

And just to be sure: I'm no expert on the caching extensions, so I can't comment on the other proposals. Throwing a dedicated exception in case of remote cache failure certainly sounds like a good idea, though.

geoand commented 1 year ago

2. definitely makes a lot of sense. What exceptions are currently thrown?

AndreasPetersen commented 1 year ago

I think it just throws whatever caused an error. For example when Redis is not available, a ConnectException is thrown.