Java fine-grained synchronization in getter/setter methods and singleton pattern of a class

Question

I'm trying to use the synchronization java directive to implement fine-grained synchronization in a class, i.e. synchronize the least amount of code I can.. I'll comment the code inline, to explain what I do and after the code I'll ask you how to improve the code:

public class MyClass {
    private static volatile MyClass singletonInstance = null;

    private HashMap<String, Integer> mHashMap = null;
    private String mStringA = null;
    private String mStringB = null;


     // Use double check technique to use synchronization only 
     // at the first getInstance() invocation
    public static MyClass getInstance() {
        if (singletonInstance == null) {
            synchronized (MyClass.class) {
                if (singletonInstance == null)
                    singletonInstance = new MyClass();
                    // Initialize class member variables
                    singletonInstance.mHashMap = new HashMap<String,Integer>();
                    singletonInstance.mStringA = new String();
                    singletonInstance.mStringB = new String();
            }

        }
        return singletonInstance;
    }

    // The following two methods manipulate the HashMap mHashMap
    // in a secure way since they lock the mHashMap instance which
    // is always the same and is unique
    public Integer getIntegerFromHashmap(String key) {
        synchronized (mHashMap) {
            return mHashMap.get(key);
        }
    }

    public void setIntegerIntoHashmap(String key, Integer value) {
        synchronized (mHashMap) {
            mHashMap.put(key, value);
        }
    }

    // With the two String members mStringA and mStringB the problem is 
    // that the instance of String pointed by the member is varied by the 
    // setter methods, so we can not lock in a fine grained way and we
    // must lock on the singletonInstance.
    public String getStringA() {
        synchronized (singletonInstance) {
            return mStringA;
        }
    }

    public String getStringB() {
        synchronized (singletonInstance) {
            return mStringB;
        }
    }

    public void setStringA(String newString) {
        synchronized (singletonInstance) {
            mStringA = newString;
        }
    }

    public void setStringB(String newString) {
        synchronized (singletonInstance) {
            mStringB = newString;
        }
    }
}

What I don't like about the getter and setter methods of the two String member variables is that locking on singletonInstance can make a thread trying to access mStringB wait until a thread that is manipulating mStringA releases its lock. What will you do in this case? Would you create two member variables like private final Integer mStringALock = new Integer(0) and private final Integer mStringBLock = new Integer(0) in MyClass and use them in the synchronized block of the getter and setter methods of mStringA and mStringB, respectively?

If you have some ideas about how to improve the above code and the proposed variation for fine-grained synchronization of the String member variables, you are welcome :)

Answer 1

Often simpler solutions are easier to implement. I would also use the concurrent library adding in 2004.

This doesn't require explicit locks and each container is thread safe.

You can use AtomicReference but in this case it doesn't give you anything volatile doesn't give you already. (As kdgregory pointed out) You might use AtomicReference in more complex cases.

public enum MyClass {
    INSTANCE;

    private final Map<String, Integer> mHashMap = new ConcurrentHashMap<String, Integer>();
    private volatile String mStringA = null;
    private volatile String mStringB = null;


    // The following two methods manipulate the HashMap mHashMap
    // in a secure way
    public Integer getIntegerFromHashmap(String key) {
        return mHashMap.get(key);
    }

    public void setIntegerIntoHashmap(String key, Integer value) {
        mHashMap.put(key, value);
    }

    public String getStringA() {
        return mStringA;
    }

    public String getStringB() {
        return mStringB;
    }

    public void setStringA(String newString) {
        mStringA = newString;
    }

    public void setStringB(String newString) {
        mStringB = newString;
    }
}

Answer 2

Yes, you need the two separate locks if you want the threads to be able to call the two methods concurrently. I would say there is nothing to improve about this.

However, I noticed that your getInstance() method tries to minimize the size of the block to synchronize but you actually don't achieve that, i.e. you check that the singletonInstance == null inside the synchronized block too. So, i think it would be better to qualify the entire method with synchronized.

It shortens the code of that method and it makes it a little bit more natural too.

Answer 3

Where to begin ...

OK, double-checked locking: it's broken (still), don't use it. If you feel you must use a singleton (and really, they're a bad idea in general, use dependency injection instead), then synchronize the getter method, and return quickly. The likelihood of contested synchronization in this case is very low unless you have a truly enormous number of cores (as in, thousands) and are constantly calling the getter method.

Replace the HashMap with a ConcurrentHashMap. Doug Lea is better at concurrent coding than either you or me.

Mark the string variables as volatile and don't synchronize them.