ConcurrentHashMap of Future and double-check locking

Question

Given:

• A lazy initialized singleton class implemented with double-check locking pattern with all the relevant volatile and synchronized stuff in getInstance. This singleton launches asynchronous operations via an ExecutorService,
• There are seven type of tasks, each one identified by a unique key,
• When a task is launched, it is stored in a cached based on ConcurrentHashMap,
• When a client ask for a task, if the task in the cache is done, a new one is launched and cached; if it is running, the task is retrieved from the cache and passed to the client.

Here is a excerpt of the code:

private static volatile TaskLauncher instance;
private ExecutorService threadPool;
private ConcurrentHashMap<String, Future<Object>> tasksCache;

private TaskLauncher() {
    threadPool = Executors.newFixedThreadPool(7);
    tasksCache = new ConcurrentHashMap<String, Future<Object>>();
}

public static TaskLauncher getInstance() {
    if (instance == null) {
        synchronized (TaskLauncher.class) {
            if (instance == null) {
                instance = TaskLauncher();
            }
        }
    }
    return instance;
}

public Future<Object> getTask(String key) {
    Future<Object> expectedTask = tasksCache.get(key);
    if (expectedTask == null || expectedTask.isDone()) {
        synchronized (tasksCache) {
            if (expectedTask == null || expectedTask.isDone()) {
                // Make some stuff to create a new task
                expectedTask = [...];
                threadPool.execute(expectedTask);
                taskCache.put(key, expectedTask);
            }
        }
    }
    return expectedTask;
}

I got one major question, and another minor one:

1. Do I need to perform double-check locking control in my getTask method? I know ConcurrentHashMap is thread-safe for read operations, so my get(key) is thread-safe and may not need double-check locking (but yet quite unsure of this…). But what about the isDone() method of Future?
2. How do you chose the right lock object in a synchronized block? I know it must no be null, so I use first the TaskLauncher.class object in getInstance() and then the tasksCache, already initialized, in the getTask(String key) method. And has this choice any importance in fact?

Answer 1

Do I need to perform double-check locking control in my getTask method?

You don't need to do double-checked locking (DCL) here. (In fact, it is very rare that you need to use DCL. In 99.9% of cases, regular locking is just fine. Regular locking on a modern JVM is fast enough that the performance benefit of DCL is usually too small to make a noticeable difference.)

However, synchronization is necessary unless you declared tasksCache to be final. And if tasksCache is not final, then simple locking should be just fine.

I know ConcurrentHashMap is thread-safe for read operations ...

That's not the issue. The issue is whether reading the value of the taskCache reference is going to give you the right value if the TaskLauncher is created and used on different threads. The thread-safety of fetching a reference from a variable is not affected one way or another by the thread-safety of the referenced object.

But what about the isDone() method of Future?

Again ... that has no bearing on whether or not you need to use DCL or other synchronization.

For the record, the memory semantics "contract" for Future is specified in the javadoc:

"Memory consistency effects: Actions taken by the asynchronous computation happen-before actions following the corresponding Future.get() in another thread."

In other words, no extra synchronization is required when you call get() on a (properly implemented) Future.

How do you chose the right lock object in a synchronized block?

The locking serves to synchronize access to the variables read and written by different threads while hold the lock.

In theory, you could write your entire application to use just one lock. But if you did that, you would get the situation where one thread waits for another, despite the first thread not needing to use the variables that were used by the other one. So normal practice is use a lock that is associated with the variables.

The other thing you need to be ensure is that when two threads need to access the same set of variables, they use the same object (or objects) as locks. If they use different locks, then they don't achieve proper synchronization ...

(There are also issues about whether lock on this or on a private lock, and about the order in which locks should be acquired. But these are beyond the scope of the question you asked.)

Those are the general "rules". To decide in a specific case, you need to understand precisely what you are trying to protect, and choose the lock accordingly.

Answer 2

1. AbstractQueuedSync used in side FutureTask has a variable state of a thread and its a volatile (thread safe) variable. So need not to worry about isDone() method.

   private volatile int state;

2. Choice of lock object is based on the instance type and situation, Lets say you have multiple objects and they have Sync blocks on TaskLauncher.class then all the methods in all the instances with be synchronized by this single lock (use this if you want to share a single shared memory across all the instances).

If all instances have their own shared memory b/w threads and methods use this. Using this will save you one extra lock object as well. In your case you can use TaskLauncher.class ,tasksCache, this its all same in terms of synchronization as its singelton.