jasypt-spring-boot

jasypt-spring-boot copied to clipboard

The original code:

public class CachingDelegateEncryptablePropertySource<T> ...
    ...
    private final Map<String, Object> cache;
    ...

    @Override
    public Object getProperty(String name) {
        // Can be called recursively, so, we cannot use computeIfAbsent.
        if (cache.containsKey(name)) {
            return cache.get(name);
        }
        synchronized (name.intern()) {
            if (!cache.containsKey(name)) {
                Object resolved = getProperty(resolver, filter, delegate, name);
                if (resolved != null) {
                    cache.put(name, resolved);
                }
            }
            return cache.get(name);
        }
    }

    @Override
    public void refresh() {
        log.info("Property Source {} refreshed", delegate.getName());
        cache.clear();
    }
}

If we want to use HashMap under multi-thread, then whether it is a read method or a write method, it needs to be protected with a synchronized block or lock. In the refresh method, we directly call the clear method of HashMap, which is not thread-safe. In addition, in the getProperty method, using synchronized (name.intern()) to achieve thread safety is actually risky. The same lock must be used to ensure that each thread can access the code protected by the lock mutually exclusive. If there are multiple locks , there is still a risk of reentrancy.

In this scenario, using ConcurrentHashMap directly is still a good choice.

In addition, considering that the main purpose of this cache is to reduce the cost of decryption, I changed the way of thinking to solve the problem that the new value cannot be obtained when the property is updated (in the original solution, the cache was not cleared in time), the new solution no longer rely on cache clearing, and even the related refresh code can be removed.

Design idea of ​​the new solution: when caching data, store the original property value and decryption result together. When reading the property, first obtain the original property value through the delegate.getProperty. If there is a corresponding cache, compare the two original values. If they are the same, return the cached decryption result, otherwise recalculate the decryption result, which ensures that when the property value changes, it can be automatically recalculated instead of using the cached old data.

After my test, this solution can solve the problem that the hot update of com.ctrip.framework.apollo : apollo-client configuration does not take effect.

如果我们想在多线程下使用HashMap，那么不管是读方法还是写方法，都需要用同步块或锁保护起来。在refresh方法中，我们直接调用了HashMap的clear方法，这是不安全的。另外在getProperty方法中，使用synchronized (name.intern())来达到线程安全，实际上也存在风险，必须用同一把锁才能保证各线程互斥地访问被锁保护的代码，如果是多把锁，则仍存在重入的风险。

此场景下，直接使用ConcurrentHashMap仍旧是一个不错的选择。

另外，考虑到本缓存的主要目的是减少解密的成本，所以我换了一种思路，来解决属性更新时无法取到新值的问题（在原方案中，缓存没有被及时的清除），新方案不再依赖于主动清除缓存，甚至可以移除掉相关的刷新代码。

新方案设计思路：缓存数据的时候，把原始属性值和解密结果一同存起来，在读取属性的时候，先通过delegate.getProperty获取原始属性值，如果存在存对应的缓存，则先比较两个原始值是否相同，如果相同则返回缓存的解密结果，否则重新计算解密结果，这样便保证了当属性值发生变更时，能自动重新计算，而不是使用缓存的旧数据。

经过我的测试，此方案，可以解决com.ctrip.framework.apollo : apollo-client配置热更新不生效的问题。

ConcurrentHashMap have bugs and fix in Java9,Java8 can`t use ConcurrentHashMap

@skysliently

ConcurrentHashMap have bugs and fix in Java9,Java8 can`t use ConcurrentHashMap

I guess the bug you mentioned is that the map cannot be updated in the computeIfAbsent method of ConcurrentHashMap in java8, otherwise deadlock may occur.

But in the new solution I provided, the methods of computeIfAbsent are not used, so there should be no problem

If I guess wrong, please describe the bug you said

我猜测你所说的bug指的是java8中ConcurrentHashMap的computeIfAbsent方法内不能更新map，否则可能会出现死锁。

但是在我提供的新解决方案中，并没有使用computeIfAbsent这些方法，那应该是没有问题的

如果我猜错了，麻烦描述一下你说的bug

@trytocatch Using delegate. getProperty to obtain the new value，CachingDelegateEncryptablePropertySource has become meaningless?

I don't think so, the purpose of this cache is to reduce the cost of decryption, if the property not changed, the cached decryption result will be used. Get the latest value every time through delegate.getProperty(usually very fast), whitch can avoid the problem of getting the old value of the cache

---- Replied Message ---- | From | @.> | | Date | 07/20/2023 11:35 | | To | @.> | | Cc | @.>@.> | | Subject | Re: [ulisesbocchio/jasypt-spring-boot] Solve: the dynamically updated property value cannot be obtained, and the concurrency problem (PR #334) |

@trytocatch Using delegate. getProperty to obtain the new value，CachingDelegateEncryptablePropertySource has become meaningless?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>