quarkusio / quarkus

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

Discrepancy between Quarkus REST behavior and Cache @CacheInvalidateAll behavior #39971

Open gsmet opened 2 months ago

gsmet commented 2 months ago

I have the following endpoint using Quarkus REST (I was trying to update from RESTEasy Classic):

    @GET
    @Produces(MediaType.TEXT_HTML)
    @CacheInvalidateAll(cacheName = CacheNames.MILESTONES_CACHE_NAME)
    public TemplateInstance index() throws IOException {
        return Templates.index(gitHub.getOpenMilestones());
    }

Note the TemplateInstance return type.

When I load this resource, I get:

java.lang.IllegalStateException: The current thread cannot be blocked: vert.x-eventloop-thread-1
    at io.smallrye.mutiny.operators.uni.UniBlockingAwait.await(UniBlockingAwait.java:30)
    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.CacheInvalidateAllInterceptor.invalidateAllBlocking(CacheInvalidateAllInterceptor.java:74)
    at io.quarkus.cache.runtime.CacheInvalidateAllInterceptor.intercept(CacheInvalidateAllInterceptor.java:38)
    at io.quarkus.cache.runtime.CacheInvalidateAllInterceptor_Bean.intercept(Unknown Source)

Which seems in line with the code in CacheInvalidateAllInterceptor:

    @AroundInvoke
    public Object intercept(InvocationContext invocationContext) throws Exception {
        CacheInterceptionContext<CacheInvalidateAll> interceptionContext = getInterceptionContext(invocationContext,
                CacheInvalidateAll.class, false);

        if (interceptionContext.getInterceptorBindings().isEmpty()) {
            // This should never happen.
            LOGGER.warn(INTERCEPTOR_BINDINGS_ERROR_MSG);
            return invocationContext.proceed();
        }
        ReturnType returnType = determineReturnType(invocationContext.getMethod().getReturnType());
        if (returnType == ReturnType.NonAsync) {
            return invalidateAllBlocking(invocationContext, interceptionContext);

        } else {
            return invalidateAllNonBlocking(invocationContext, interceptionContext, returnType);
        }
    }

// ...

    protected static ReturnType determineReturnType(Class<?> returnType) {
        if (Uni.class.isAssignableFrom(returnType)) {
            return ReturnType.Uni;
        }
        if (CompletionStage.class.isAssignableFrom(returnType)) {
            return ReturnType.CompletionStage;
        }
        return ReturnType.NonAsync;
    }

AFAICS, Quarkus REST decides to make the endpoint reactive and CacheInvalidateAllInterceptor decides that it's blocking and thus there's a mismatch in behavior.

I'm a bit unsure how we should fix this. I'm also surprised that we don't consider the @Blocking/@NonBlocking annotations in this code. And same question for handling Multi?

I worked around it for now by adding a @Blocking annotation to the endpoints.

quarkus-bot[bot] commented 2 months ago

/cc @gwenneg (cache)

gsmet commented 2 months ago

/cc @geoand too

geoand commented 2 months ago

The use of TemplateInstance forces Quarkus REST to run your method on the event loop. We made that so a while back, but given that this is not the first time I've seen this issue, I am wondering whether it was the wrong call.

cc @mkouba

Doesn't the use of @Blocking fix the issue?

gsmet commented 2 months ago

@geoand yes that's how I fixed it for now. See:

I worked around it for now by adding a @Blocking annotation to the endpoints.

But it's not an ideal experience. And there is still the question about Multi as in the case of @CacheInvalidateAll, it makes sense, I think, as you can point to a named cache that is not the one of the method.

Also, I would expect the code determining if we are reactive or not to at least have a look at the @Blocking/@NonBlocking annotations.

geoand commented 2 months ago

Also, I would expect the code determining if we are reactive or not to at least have a look at the @Blocking/@NonBlocking annotations

This is the part I'm not following, care to elaborate please?

mkouba commented 2 months ago

The use of TemplateInstance forces Quarkus REST to run your method on the event loop. We made that so a while back, but given that this is not the first time I've seen this issue, I am wondering whether it was the wrong call.

cc @mkouba

Yep, it was probably a wrong move. We should deprecate the io.quarkus.resteasy.reactive.server.spi.NonBlockingReturnTypeBuildItem. I'm not so sure if we should just (a) switch the default behavior to TemplateInstance -> blocking + update migration notes or (b) switch and provide a config property to enable the old behavior (for backrward compatibility).

@geoand WDYT?

geoand commented 2 months ago

We should deprecate the io.quarkus.resteasy.reactive.server.spi.NonBlockingReturnTypeBuildIteμ

Given it's only used in Qute (at least in the core repo), I think we can do this.

or (b) switch and provide a config property to enable the old behavior (for backrward compatibility).

This is probably safer

mkouba commented 2 months ago

We should deprecate the io.quarkus.resteasy.reactive.server.spi.NonBlockingReturnTypeBuildIteμ

Given it's only used in Qute (at least in the core repo), I think we can do this.

or (b) switch and provide a config property to enable the old behavior (for backrward compatibility).

This is probably safer

Ok, I will send a PR later today.