Synchronization in a HashMap cache

One possible problem is that you create unnecessary contention by executing veryCostlyOperation() inside a synchronized block, so that many threads cannot retrieve their (independent) resources at the same time. This can be solved by using Future<Resource> as values of the map:

Map<String, Future<Resource>> map = new ConcurrentHashMap<String, Future<Resource>>();    ...Future<Resource> r = map.get(name);if (r == null) {    FutureTask task = null;    synchronized (lock) {        r = map.get(name);        if (r == null) {            task = new FutureTask(new Callable<Resource>() {                public Resource call() {                    return veryCostlyOperation(name);                }            });            r = task;            map.put(name, r);        }    }    if (task != null) task.run(); // Retrieve the resource}return r.get(); // Wait while other thread is retrieving the resource if necessary

The only potential problem I see is that you synchronize to this. If any other code in the same class also synchronizes to this, only one of those blocks will run at once. Maybe there's nothing else that does this, and that's fine. I always worry about what the next programmer is going to do, though. (or myself in three months when I've forgotten about this code)

I would recommend creating a generic synch object and then synch'ing to that.

private final Object resourceCreationSynchObject = new Object();

then

synchronized(this.resourceCreationSynchObject) {  ...}

Otherwise, this does exactly what you're asking for. It ensures that veryCostlyOperation cannot be called in parallel.

Also, it's great thinking to re-get the resource a second time within the synchronized block. This is necessary, and the first call outside makes sure that you don't synchronize when the resource is already available. But there's no reason to call it a third time. First thing inside the synchronized block, set resource again to resources.get(name) and then check that variable for null. That will prevent you from having to call get again inside the else clause.

Your code looks ok, except that you are synchronizing more than actually required:

• Using a ConcurrentHashMap instead of a synchronized HashMap would allow multiple invocations of the get method without locking.

• Synchronizing on this instead of resources is probably not necessary, but it depends on the rest of your code.