Ensure that failed unis are not cached #39762
Conversation
geoand
commented
Mar 28, 2024
•
geoand
commented
- Fixes: Quarkus caches Failures #39677
|
@cescoffier does this seems correct to you? I did a few manual tests and it does seem to behave correctly. If you agree, I can look into adding a test |
|
A test would be good. |
|
Test added |
geoand
added
the
triage/waiting-for-ci
quarkus-bot
bot
removed
the
triage/waiting-for-ci
Status for workflow
|
|
Made it clear that it was also backported to 3.9.2. |
| }).onFailure().call(new Function<>() { | ||
| @Override | ||
| public Uni<?> apply(Throwable throwable) { | ||
| return cache.invalidate(key).replaceWith(throwable); |
There was a problem hiding this comment.
I was considering backporting this one and I have a question: my understanding is that we invalidate the cache key if we have an error?
I would expect us to not store anything in this case? And thus we wouldn't have to invalidate the cache? I'm especially worried in the case of concurrent accesses because I wouldn't expect us to invalidate a cache entry that could have been stored from another thread.
Now it's reactive code so I'm not understanding exactly what it does but this concerns me a bit.
There was a problem hiding this comment.
Your understanding is correct, but given the API, I am not sure how it can be done differently
There was a problem hiding this comment.
Yeah my problem is that again IIUC we have a very short period of time when the error is cached so another thread can get it.
Also we have a short period of time when we might invalidate an actually valid key but this is probably more theoretical (as I wouldn't expect another thread to store a cache entry) and less problematic.
There was a problem hiding this comment.
Yes, that's certainly true
There was a problem hiding this comment.
@gwenneg could we have some feedback from you here? Thanks!
There was a problem hiding this comment.
@gsmet, you are right. There is a chance to have concurrent access and replace a correct value. Reactive or not does not change anything here.
For Redis, we could imagine using a Redis transaction (be careful, it is not a database transaction). For others, I'm not sure. We could imagine adding an atomic operation doing this.
