TransactionAwareCacheDecorator renders CacheErrorHandler useless

[neiser]

Affects: All Versions since 3.2?

When configuring a custom cache error handler via CachingConfigurer::errorHandler and having transaction-aware caching enabled (for example, via RedisCacheManager.RedisCacheManagerBuilder::transactionAware, which decorates with TransactionAwareCacheDecorator), the cache error handler is never called when the put fails in TransactionSynchronization::afterCommit once the transaction was committed. In particular, this prevents users to suppress runtime exceptions from the cache backend by using the LoggingCacheErrorHandler, such as connection problems or command timeouts from Redis.

To illustrate the problem, I've created a simple demo project using Redis as a cache backend (which cannot connect as there's no Redis running on localhost:6379)

The test where I did not enable transaction-awareness does not throw any exception, whereas the test with transaction-awareness does, rather unexpectedly, as I've installed a LoggingCacheErrorHandler.

Note that I've configured a very dummy transaction handling to make the bug appear.

A workaround for the bug would be to not enable transaction-awareness via RedisCacheManager.RedisCacheManagerBuilder::transactionAware, but to instrument the cache manually. I did this with AOP on any cache instance by decorating the CacheManager::getCache with a BeanPostProcessor, but this is quite ugly:

@Aspect
@RequiredArgsConstructor
@EqualsAndHashCode
public class CacheTransactionAwareAspect {
    private final CacheErrorHandler cacheErrorHandler;
    private final Cache cache;

    private static Object proceedAfterCommit(ProceedingJoinPoint pjp, Consumer<RuntimeException> errorHandler) throws Throwable {
        if (TransactionSynchronizationManager.isSynchronizationActive()) {
            TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
                @Override
                public void afterCommit() {
                    try {
                        pjp.proceed();
                    } catch (RuntimeException e) {
                        errorHandler.accept(e);
                    } catch (Throwable e) {
                        throw new RuntimeException(e);
                    }
                }
            });
            return null;
        } else {
            return pjp.proceed();
        }
    }

    @Around("execution(* org.springframework.cache.Cache.put(..))")
    public Object wrapPutMethod(ProceedingJoinPoint pjp) throws Throwable {
        return proceedAfterCommit(pjp, e -> {
            var args = pjp.getArgs();
            cacheErrorHandler.handleCachePutError(e, cache, args[0], args[1]);
        });
    }

    @Around("execution(* org.springframework.cache.Cache.evict(..))")
    public Object wrapEvictMethod(ProceedingJoinPoint pjp) throws Throwable {
        return proceedAfterCommit(pjp, e -> {
            var args = pjp.getArgs();
            cacheErrorHandler.handleCacheEvictError(e, cache, args[0]);
        });
    }

    @Around("execution(* org.springframework.cache.Cache.clear(..))")
    public Object wrapClearMethod(ProceedingJoinPoint pjp) throws Throwable {
        return proceedAfterCommit(pjp, e -> {
            cacheErrorHandler.handleCacheClearError(e, cache);
        });
    }
}

Let me know if you need further information to reproduce the bug.

I've also just tried a fix, but I don't know how to get the error handler (which should be kind of a singleton from CachingConfigurer) into the AbstractTransactionSupportingCacheManager. Any hints would be appreciated and I'd create a PR if this attempt goes into the right direction.

[sbrannen]

When you say CachingCustomizer, do you instead mean org.springframework.cache.annotation.CachingConfigurer?

[neiser]

@sbrannen Ah yes, I meant CachingConfigurer. I've updated the description and the example code.

[neiser]

@sbrannen Is there any update from your side? Anything I could do to support you with this?

[mAlaliSy]

Any update on this?

[neiser]

Maybe @rstoyanchev can explain why this has been removed from the Triage Queue without further comment? 🤔

[sbrannen]

We deleted the triage queue milestone in favor of a different internal process.

Note that this issue is still assigned the “waiting-for-triage” label. So its status has not changed.