0

In our web application exists 'Access Bean' classes with some methods on them which perform queries on a DB, these are invoked multiple times per request. Therefore they need to be cached but the original classes cannot be altered as they are from the framework we use, so to work around this, I have created a factory which builds a proxy class to wrap and cache method calls to original existing bean methods. I want to store the proxy class in a ConcurrentHashMap<Class, Class> (where the key is the original class and the value is the new proxy class) to be quickly retrieved by other threads when needed.

I want it to be fast and with very low contention, so I have to synchronized on the bean class object passed in as a method parameter for double checked locking to get the proxy class from the map or controlling access to the proxy factory if the proxy class is missing.

Map<Class, Class> classProxyMap = ConcurrentHashMap<Class, Class>;

@SuppressWarnings("unchecked")
private static <P extends B> Class<P> getProxyClass(Class<B> beanClass) {

Class<P> proxyClass = classProxyMap.get(beanClass);

    if (proxyClass == null) {
        synchronized (beanClass) {
            proxyClass = classProxyMap.get(beanClass);
            if (proxyClass == null) {
                proxyClass = proxyFactory.makeProxyClass(beanClass, false);
                classProxyMap.put(beanClass, proxyClass);
            }
        }
    }

    return (Class<P>) proxyClass;
}

It seems as though this is failing in a QA environment, although I am not entirely sure this (synchronization) is the issue. Therefore I want to know if it is ok to synchronize in this way or should I just synchonize on the map and be done with it.

[EDIT] The Application server is Java 7 so ConcurrentHashMap.computeIfAbsent is not an option

lifesoordinary
  • 123
  • 2
  • 9
  • 4
    Synchronizing on external things is dangerous, especially since the `Class` is used as lock for `static synchronized` methods in that class. I also doubt you'll gain a lot by adding this synchronized block because if there happens to be another thread that creates a proxy in the short time it takes to create one you'd probably live without issues. You could use `ConcurrentMap#putIfAbsent` in Java7 without extra locks if the risk of a few duplicate proxies is ok. Otherwise just synchronize on something you "own". – zapl Jan 21 '19 at 22:30
  • AFAIK There should only ever be one instance of Class (like enum) so I believe this should be safe. Is there any chance that synchonizing like this on the class itself could fail? Please give an example if you can. I do realize it might not be worth the speed up. – lifesoordinary Jan 21 '19 at 22:38
  • 1
    @lifesoordinary "Synchronizing on external things is dangerous" the point is that the same instance of `Class` can be obtained and synchronized on elsewhere in your code, which could lead to contention here. – Andy Turner Jan 21 '19 at 23:02
  • Double-checked locking is flawed, unnecessary, and if done wrong, ineffective. Yours is done wrong because the first null check on the lock object is unguarded by any thread coordination, so race-condition risk is rampant. – Lew Bloch Jan 22 '19 at 03:28
  • @LewBloch this should actually be ok here since `proxyClass` comes from a `ConcurrentHashMap` which does all the thread coordination & visibility guarantees you'd typically use a `volatile` for. – zapl Jan 22 '19 at 03:47
  • After you retrieve `proxyClass` you do an unprotected check on the returned value. During that time another thread can access the map, but that won't change the value of `proxyClass`. It's a classic check-and-set race condition. – Lew Bloch Jan 29 '19 at 20:05
  • Also, synchronization on `beanClass` is unrelated to the map's concurrency mechanism. – Lew Bloch Jan 29 '19 at 20:06

1 Answers1

0

What for do you need extra synchronization if you already have ConcurrentHashMap?

Why not to use existing ConcurrentHashMap#computeIfAbsent?

Map<Class, Class> classProxyMap = ConcurrentHashMap<Class, Class>;

@SuppressWarnings("unchecked")
private static <P extends B> Class<P> getProxyClass(Class<B> beanClass) 
{
    return classProxyMap.computeIfAbsent(
            beanClass, 
            (clazz) -> proxyFactory.makeProxyClass(clazz, false));
}
Dmitry Zvorygin
  • 473
  • 6
  • 14