Jens Wilke opened SPR-17052 and commented

Code from CacheAspectSupport:

// Special handling of synchronized invocation
if (contexts.isSynchronized()) {
...
    try {
        return wrapCacheValue(method, cache.get(key, () -> {
            return unwrapReturnValue(invokeOperation(invoker));
        }));
    }
    catch (Cache.ValueRetrievalException ex) {
        // The invoker wraps any Throwable in a ThrowableWrapper instance so we
        // can just make sure that one bubbles up the stack.
        throw (CacheOperationInvoker.ThrowableWrapper) ex.getCause();
    }
}
  • In the code path for @Cachable(sync=true) CacheErrorHandler is not visited.
  • cast to (CacheOperationInvoker.ThrowableWrapper) is not totally sane, it could be omitted since then the unwrapping would happen in CacheInterceptor.invoke

Affects: 5.0.7

Comment From: roborobo2

I have the same problem in spring v4.3.13

Comment From: danielmzak

and the same problem in Spring ver. 5.1.6

Comment From: snicoll

Things aren't as easy to implement I am afraid. The purpose of the listener is to determine if the exception should bubble up the stack or if we should always call the underlying method and ignore it.

With a sync call, the cache library is responsible to check its internal state and call the underlying method if necessary (and sync other threads attempting to lookup the same value). If we call the error handler on get and your custom implementation decides to ignore it, then we should call the method and the value will not be stored in the cache. So the next thread waiting for that key (potentially) is likely to call the method again if the cache hadn't recover. There is also the odd situation that we rely on CacheRetrievalException to identify if the exception was thrown by the caller or the cache provider.

All in all, we are less in control and less likely to provide the right semantics. I've pushed some initial work that calls the error handler in such case, flagging for team attention to see what the rest of the team thinks.

Comment From: rmustard

same problem in 5.2.1

I'm using error handler to ignore scenarios where Redis is unavailable.

Is there a recommended workaround for sync=true and handling get errors?

Comment From: trim09

I would like to use an error handler to invalidate cache on deserialization exception during a new deployment even though Cacheable has sync=true attribute. 

    @Override
    public void handleCacheGetError(RuntimeException e, Cache cache, Object o) {
        if (e instanceof HazelcastSerializationException) {
            log.error(e.getMessage(), e);
            cache.invalidate();
        }
    }

Comment From: mstawick

A lot of digging until I discovered that it's sync=true that's the cause of my trouble. Have you guys found any viable workaround, or is disabling sync the only way?

Comment From: trim09

TLDR: Unfortunately, I do not have any workaround for that.

I used to tear down and spin up again whole cluster whenever we introduce any incompatible cache item that cause HazelcastSerializationException. :-/ I know I could contribute to this cool open source project, but I have not done so on this issue yet.

Comment From: chrisirhc

I've pushed some initial work that calls the error handler in such case, flagging for team attention to see what the rest of the team thinks.

This seems like a reasonable fix for this issue. Are there any downsides of this approach? Or arguments against this fix that prevent it from going into a PR state? The only thing I can think of is to wrap the exception in a new exception type or to add a configuration option to either opt-in or opt-out of this new behaviour ( https://github.com/snicoll/spring-framework/commit/68e365644f61183d03df39ad780d081afd3b5d75#r133464876 ) .

I was looking into this and realised I would've ended up making something very similar to this same fix.

Happy to roll this into a PR with the above additionals, if there's interest, and consideration, and some buy-in from the team on this fix.

The downside of not having such a fix is that every caller of the Cacheable would need to have its own error handling mechanism to catch such a scenario, which would likely be to attempt to make the same call.

Comment From: adamcamp

Is there any update on this one? It seems like a pretty common use case to ignore cache failures and continue with the method call, but that is currently not possible with sync=true setting.

The only workaround I've found while still using Cacheable annotation is to set sync=false, but that is also not ideal from performance perspective.

I understand that the sync=true would not be honored if the cache is down, but that is fine from my perspective. The purpose of sync=true in my use case is that when the cache is up it should wait for the value. If the cache is down I'm perfectly fine with all the method calls going through in parallel.