Support non-blocking cache invalidation #22683

gwenneg · 2022-01-06T09:59:46Z

Based on @stuartwdouglas' suggestion in the Quarkus mailing-list.

The cache invalidation is now possible with resteasy-reactive as long as the method annotated with @CacheInvalidate or @CacheInvalidateAll returns a Uni. It means that the following code will work:

@Path("/value")
public class ReactiveResource {
    @GET
    @CacheInvalidate(cacheName = "mycache")
    public Uni<Void> getValue() {
        return Uni.createFrom().nullItem();
    }
}

On the other hand, this one will keep blocking the event loop thread and should not be used with resteasy-reactive:

@NonBlocking
@Path("/value")
public class ReactiveResource {
    @GET
    @CacheInvalidate(cacheName = "mycache")
    public void getValue() {
    }
}

⚠️ I am preparing an update for the quarkus-cache doc related to the integration between the extension and Mutiny. It will mention this as well as other important things. I'll create a separate PR for it if that's OK.

gwenneg · 2022-01-06T10:02:27Z

cc @rsvoboda

gsmet · 2022-01-07T11:51:46Z

@mkouba would you mind reviewing this one? I think you're the best person for it given Stuart is not around.

rsvoboda · 2022-01-07T11:56:38Z

Just FYI, played with the revert.50d90aa branch of quarkus-test-suite and using Uni<Void> as return type for reactive endpoints makes the tests pass when executed against patched main.

mkouba · 2022-01-07T13:08:30Z

extensions/cache/deployment/src/test/java/io/quarkus/cache/test/runtime/UniReturnTypeTest.java

+    void testCacheResult() {
+        // STEP 1
+        // Action: a method annotated with @CacheResult and returning a Uni is called.
+        // Expected effect: the method is not invoked, as Uni is lazy.


I think that I understand the reasoning but I find this rather counter-intuitive. As the method is annotated with @CacheResult I would expect the method to be executed but the result of the invocation to be cached. But I'm no reactive expert so it might be perfectly OK.

@cescoffier @jponge WDYT?

Everything (invocation and caching of the result) happens at subscription time. This PR doesn't change that part of the extension BTW. I refactored CacheResultInterceptor a bit to get rid of runtime lambdas, nothing more.

Yep, I'm not saying that it's a change or anything but this is the first time I see it... and it's not documented anyway so ;-)

It was changed recently though with #22271.

and it's not documented anyway so ;-)

It will be shortly, I'm on it 😄

I would naively expect that the method is invoked, the result is cached and a next invocation would return a new Uni for the cached result. But like I said - I'm no expert in this area at all.

That always puzzled me.
With:

@CahcedResult Uni<Person> getPerson() { ... }

What is cached? The Uni? The emitted Person?

If you cache the Uni, resubscribing will re-execute the logic, so probably not what we want to achieve.
Caching the person makes more sense to me, but I'm happy to discuss it.

The original Uni is never cached by the extension. What is cached is an UnresolvedUniValue at first which is then replaced with the actual item emitted by the original Uni once it's been resolved. After that, when the cached value is accessed, a Uni.createFrom().item(theResolvedValue) is returned.

This is another part of the doc update I'm working on 😄

In other words, the method body is not called until someone subscribes to the Uni which is returned by the interceptor, right? That probably makes sense.

My naive expectation was that the method is called and once someone subscribes to the returned Uni a Uni.createFrom().item(theResolvedValue) is returned. That's very similar but I assume the current approach makes more sense from caching POV.

In other words, the method body is not called until someone subscribes to the Uni which is returned by the interceptor, right? That probably makes sense.

Exactly.

My naive expectation was that the method is called and once someone subscribes to the returned Uni a Uni.createFrom().item(theResolvedValue) is returned. That's very similar but I assume the current approach makes more sense from caching POV.

The method was invoked before the subscription until recently but it caused a bug with resteasy-reactive (blocked thread) which was fixed by @stuartwdouglas.

mkouba · 2022-01-10T14:04:02Z

extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/CacheInvalidateInterceptor.java

+    private Object invalidateNonBlocking(InvocationContext invocationContext,
+            CacheInterceptionContext<CacheInvalidate> interceptionContext) {
+        // Because of this array, the key will only be built once and reused by each iteration of the following Multi.
+        Object[] key = new Object[1];


Could you be more specific? Why the array? Is it just a holder of a reference that can be only set on the same thread?

Yes it's a holder reference to "hack" the lambda restriction about final variables. The idea is to avoid recalculating the same key each time an invalidation is performed because the key value will always be the same.

I'm not entirely sure, but the interceptor execution may involve several threads as Merge implies concurrency in Mutiny. But the key value will always be the same and if it's not available to a thread (null) then it will be calculated.

If so I'd say that AtomicReference is a little bit more expensive because of the volatile field... but also a bit less "hacky" IMO :-)

After re-reading the extension code, I think there's an issue (unrelated with the holder reference) with reusing the same key...

I just fixed a bug with the reusable cache key during an invalidation.

When a method annotated with @CacheInvalidate does not have any arguments, the cache name is used to build a default key. Reusing the same key means that the following code won't work as one of the caches won't be invalidated. It's not a common scenario as most cached methods have arguments, but still a bug (old one) 😃

@CacheInvalidate(cacheName = "foo") @CacheInvalidate(cacheName = "bar") public void invalidate() { }

Thanks for questioning the holder reference, it made me realize this 😄 The hack is gone now.

I'll add a test for this case in a few minutes.

Test added.

rsvoboda · 2022-01-11T20:34:46Z

@cescoffier / @mkouba does this PR have green light from you now?

gsmet

Let's get this in. AFAICS, the remaining discussions are more general than related to this specific PR.

quarkus-bot bot added the area/cache label Jan 6, 2022

gwenneg force-pushed the issue-22431-cache-invalidate-non-blocking branch from 66c53b9 to 219cd1c Compare January 6, 2022 10:17

mkouba reviewed Jan 7, 2022

View reviewed changes

gwenneg mentioned this pull request Jan 7, 2022

Skip caching Multi and log WARN message at build time #16706

Merged

mkouba reviewed Jan 10, 2022

View reviewed changes

gwenneg force-pushed the issue-22431-cache-invalidate-non-blocking branch from 219cd1c to 0bc0c69 Compare January 10, 2022 14:35

Support non-blocking cache invalidation

0a153c8

gwenneg force-pushed the issue-22431-cache-invalidate-non-blocking branch from 0bc0c69 to 0a153c8 Compare January 10, 2022 14:53

gsmet approved these changes Jan 12, 2022

View reviewed changes

gsmet merged commit 2e4a786 into quarkusio:main Jan 12, 2022

quarkus-bot bot added this to the 2.7 - main milestone Jan 12, 2022

gwenneg deleted the issue-22431-cache-invalidate-non-blocking branch January 12, 2022 14:44

gwenneg mentioned this pull request Jan 24, 2022

IllegalStateException: The current thread cannot be blocked using quarkus-cache with quarkus-resteasy-reactive #22251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support non-blocking cache invalidation #22683

Support non-blocking cache invalidation #22683

gwenneg commented Jan 6, 2022

gwenneg commented Jan 6, 2022

gsmet commented Jan 7, 2022

rsvoboda commented Jan 7, 2022

mkouba Jan 7, 2022

gwenneg Jan 7, 2022

mkouba Jan 7, 2022

gwenneg Jan 7, 2022

gwenneg Jan 7, 2022

mkouba Jan 7, 2022

cescoffier Jan 7, 2022

gwenneg Jan 7, 2022 •

edited

Loading

mkouba Jan 10, 2022

gwenneg Jan 10, 2022 •

edited

Loading

mkouba Jan 10, 2022

gwenneg Jan 10, 2022

mkouba Jan 10, 2022

gwenneg Jan 10, 2022

gwenneg Jan 10, 2022 •

edited

Loading

gwenneg Jan 10, 2022

gwenneg Jan 10, 2022

mkouba Jan 10, 2022

rsvoboda commented Jan 11, 2022

gsmet left a comment

Support non-blocking cache invalidation #22683

Support non-blocking cache invalidation #22683

Conversation

gwenneg commented Jan 6, 2022

gwenneg commented Jan 6, 2022

gsmet commented Jan 7, 2022

rsvoboda commented Jan 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwenneg Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwenneg Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwenneg Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsvoboda commented Jan 11, 2022

gsmet left a comment

Choose a reason for hiding this comment

gwenneg Jan 7, 2022 •

edited

Loading

gwenneg Jan 10, 2022 •

edited

Loading

gwenneg Jan 10, 2022 •

edited

Loading