Locking in Highly Concurrent System

Question

I have a class in a highly concurrent system. A method getResolvedClassName() of that class can produce deadlock. So I am designing it in the following way:

public class ClassUtils {

    private static ClassUtils classUtils;

    private transient Object object = new Object();

    private synchronized Object getObjectLock() {
        return object;
    }

    public void getResolvedClassName(Class<?> clazz) {
        synchronized (getObjectLock()) {
            //some job will be done here
        }       
    }

    public synchronized static ClassUtils getInstance() {
        if(classUtils == null) {
            classUtils = new ClassUtils();
        }
        return classUtils;
    }   
}

Am I doing it in a right way? Any information will be helpful to me.

Thanks.

Edit:

public class ClassUtils {

    private static final ClassUtils classUtils = new ClassUtils();
    private ReentrantLock lock = new ReentrantLock();

    public void getResolvedClassName(Class<?> clazz) {
        lock.lock();
        //some job will be done here
        lock.unlock();      
    }

    public static ClassUtils getInstance() {        
        return classUtils;
    }   
}

Others might disagree, but I think the question is a bit too vague to permit useful answers. — NPE, Feb 23 '12 at 17:50
`private transient Object object` - shouldn't it be `final` instead? — Tomasz Nurkiewicz, Feb 23 '12 at 17:52
Make your lock `final`, not transient. Remove the getter for it (use the attribute directly in the method). Your singleton initialization method is broken, use double locked checks or an enum. See this page for more complex lock types:- http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/locks/package-summary.html — Perception, Feb 23 '12 at 17:56
agreed, it should be final, also since its private, no need for a private getter. — BillRobertson42, Feb 23 '12 at 17:57

score 3 · Answer 1 · answered Feb 23 '12 at 17:57

This question is really a little vague, I don't see the purpose of using a singleton and why synchronization is required to do some job. If it doesn't access mutable state, it doesn't need synchronization. I can only say that three locks (ClassUtils.class, ClassUtils instance, and object) are almost certainly adding unnecessary complexity. Also, as Justin noted, you should make object final, then you won't need synchronization to access it.

score 3 · Accepted Answer · edited May 23 '17 at 12:03

A few things stand out:

I don't think the transient keyword means what you think it means. That keyword has nothing to do with synchronization, and is only used when serializing a class. You might be confusing it with volatile. Incidentally, volatile is not needed here either.
Lazy initialization of your singleton is probably unnecessary. Why don't you just do private static final ClassUtils classUtils = new ClassUtils();? Then your getInstance() method does not need to be synchronized and can just return classUtils; It is also thread safe. You should also always declare singleton instances as final.
The whole situation with getObjectLock() is not needed. You can just synchronize on this (i.e. make getResolvedClassname into a synchronized method) and will be safer and cleaner.

You could also investigate the java.util.concurrent.Lock classes to see if there is something more suitable than synchronizing on an Object, which is nowadays considered poor form.

thank you very much. I have edited my question and added a code snippet. Is this now okay? — Tapas Bose, Feb 23 '12 at 18:18
@TapasBose It is, but I wouldn't use a `ReentrantLock`. It is simpler to have `private final Object lock = new Object()` and synchronize on it instead. If you insist on using `ReentrantLock`, I suggest two things: make the field final and also place the code in `getResolvedClassName` into a try-finally statement. `lock.lock()` should be the frist statement in the try block, and `lock.unlock()` should be placed into the finally block. This is done to ensure that the lock will be unlocked if some exception happens. — Malcolm, Feb 23 '12 at 19:00

score 0 · Answer 3 · answered Feb 23 '12 at 17:53

0

Your problem is a bit general. However, you can consider initializing that value as immutable. Many immutable, initialized values are threadsafe, and require no locking.

answered Feb 23 '12 at 17:53

justin

104,054
14
179
226

Locking in Highly Concurrent System

3 Answers3