Spring @CacheEvict beforeInvocation with transaction does not work

Spring Version: 5.1.7.RELEASE

When Spring transactions are enabled, the Cache operation is proxied by TransactionAwareCacheDecorator. Only the get() operation is invoked directly. The put(), evict(), and clear() operations will be delayed until after the transaction has been committed (via a custom TransactionSynchronizationAdapter.afterCommit() callback).

When username='zhangsan' is cached, the updateAndRefresh() method will return the old cached value because userRepository.getByUsername() is executed before the transaction is committed.

Even if beforeInvocation=true, CacheAspectSupport.processCacheEvicts() also only sets cache operation to ThreadLocal.

Is there is a bug for @CacheEvict(beforeInvocation = true, ...) when executing within a transaction?

class UserRepository {
    @Cacheable(value = "userCache", key = "#username")
    User getByUsername( String username );

    @CacheEvict(value = “userCache”, key = "#user.username", beforeInvocation = true)
    void save( User user );
}

class UserService {
    @Transactional
    User updateAndRefresh( User user ) {
        userRepository.save(user);
        return userRepository.getByUsername( user.getUsername() );
    }
}

CacheAspectSupport.java

private void performCacheEvict(
            CacheOperationContext context, CacheEvictOperation operation, @Nullable Object result) {

        Object key = null;
        for (Cache cache : context.getCaches()) {
            if (operation.isCacheWide()) {
                logInvalidating(context, operation, null);
                doClear(cache);
            }
            else {
                if (key == null) {
                    key = generateKey(context, result);
                }
                logInvalidating(context, operation, key);
                doEvict(cache, key);
            }
        }
    }

TransactionAwareCacheDecorator.java

@Override
    public void evict(final Object key) {
        if (TransactionSynchronizationManager.isSynchronizationActive()) {
            TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronizationAdapter() {
                @Override
                public void afterCommit() {
                    TransactionAwareCacheDecorator.this.targetCache.evict(key);
                }
            });
        }
        else {
            this.targetCache.evict(key);
        }
    }

Comment From: sbrannen

Good catch!

Is there is a bug for @CacheEvict(beforeInvocation = true, ...) when executing within a transaction?

According to its class-level Javadoc, I would say that TransactionAwareCacheDecorator behaves as expected:

Cache decorator which synchronizes its put, evict and clear operations with Spring-managed transactions (through Spring's TransactionSynchronizationManager, performing the actual cache put/evict/clear operation only in the after-commit phase of a successful transaction.

However, there is definitely a conflict here with the documentation for @CacheEvict(beforeInvocation = true, ...), which states:

Whether the eviction should occur before the method is invoked.

Setting this attribute to true, causes the eviction to occur irrespective of the method outcome (i.e., whether it threw an exception or not).

Unfortunately, it does not appear that it would be easy to make that work within a transaction given the current API for org.springframework.cache.Cache.evict(Object). Specifically, there is no way to propagate the beforeInvocation = true semantics to the Cache implementation via the current evict(Object) method, and that's basically what TransactionAwareCacheDecorator would need in order to know that it should perform the eviction immediately.

@snicoll, what are your thoughts on the matter?

Comment From: wusthuke

I tried to resolved follow, record Evict beforeInvocation to ThreadLocal :

EvictCacheThreadLocalContext.java

package org.springframework.cache;

public class EvictCacheThreadLocalContext {

    private static final ThreadLocal<Boolean> evictBeforeInvocation = ThreadLocal.withInitial(() -> false);

    public static boolean isEvictBeforeInvocation() {
        return evictBeforeInvocation.get();
    }

    public static void setEvictBeforeInvocation(boolean beforeInvocation) {
        evictBeforeInvocation.set(beforeInvocation);
    }

    public static void remove() {
        evictBeforeInvocation.remove();
    }
}

CacheAspectSupport.java

private void performCacheEvict(
            CacheOperationContext context, CacheEvictOperation operation, @Nullable Object result) {

        Object key = null;
        for (Cache cache : context.getCaches()) {
            if (operation.isCacheWide()) {
                logInvalidating(context, operation, null);
                doClear(cache);
            } else {
                if (key == null) {
                    key = generateKey(context, result);
                }
                logInvalidating(context, operation, key);

                try {
                    EvictCacheThreadLocalContext.setEvictBeforeInvocation(operation.isBeforeInvocation());
                    doEvict(cache, key);
                } finally {
                    EvictCacheThreadLocalContext.remove();
                }
            }
        }
    }

TransactionAwareCacheDecorator.java

@Override
    public void evict(final Object key) {
        if (EvictCacheThreadLocalContext.isEvictBeforeInvocation()) {
            this.targetCache.evict(key);
            return;
        }

        if (TransactionSynchronizationManager.isSynchronizationActive()) {
            TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronizationAdapter() {
                @Override
                public void afterCommit() {
                    TransactionAwareCacheDecorator.this.targetCache.evict(key);
                }
            });
        } else {
            this.targetCache.evict(key);
        }
    }

Comment From: sbrannen

Thanks for the example workaround, @wusthuke.

A ThreadLocal should work for propagating that boolean flag, but I'm not sure that we want to go that route in terms of the implementation.

We will think it over.

Comment From: jhoeller

After some back and forth, I've introduced a new boolean evictIfPresent(Object key) operation on the Cache abstraction which implies immediate execution semantics and provides an indication for whether the key was present (as supported by all cache providers, so this is also worth exposing for sophisticated programmatic interaction purposes).

In case of @CacheEvict.beforeInvocation=true, the cache interceptor delegates to evictIfPresent now: not using the returned indication but relying on immediate execution semantics (in particular the explicit pass-through in TransactionAwareCacheDecorator).

Comment From: snicoll

I've introduced a new boolean evictIfPresent(Object key) operation on the Cache abstraction which implies immediate execution semantics and provides an indication for whether the key was present

@jhoeller I don't see any commit linked to this issue and it is still open. Am I missing something?

Comment From: sbrannen

Juergen hasn't committed that change yet.

During the team call yesterday, we discussed that the proposal likely would not cover the case when allEntries and beforeInvocation are both set to true. So, it's my understanding that Juergen is further investigating the issue.

Comment From: wusthuke

Juergen hasn't committed that change yet.

During the team call yesterday, we discussed that the proposal likely would not cover the case when allEntries and beforeInvocation are both set to true. So, it's my understanding that Juergen is further investigating the issue.

@sbrannen I verified 5.2.0.RELEASE, and found that this implementation has a little problem. when cache is empty and database exists, after UserService.update(id, userInfo), cache will stored old userInfo

AbstractCacheInvoker my modify as

if (immediate) {
    cache.evictIfPresent(key);
    cache.evict(key);
}