I’m writing a proxy server that serves cached resources. If resource is not cached, it’s retrieved and put into filesystem. Server is multi-threaded and I want to avoid any kind of global locking. If there are several requests for the same resource that is not cached at the same time, one request should retrieve resource, put it into filesystem and other requests should be blocked and then serve cached resource.

My first approach was to synchronize on interned string:

void serveFile(Path file) {     if (!Files.exists(file)) {         synchronized (file.toString().intern()) {             if (!Files.exists(file)) {                 retrieveResource(file);             }         }     }     serveResource(file); } 

But there are many discussions about why this approach is not the best, so I rewrote is as follows:

private final ConcurrentMap<Path, Object> downloadLocks = new ConcurrentHashMap<>();  void serveFile(Path file) {     if (!Files.exists(file)) {         Object lock = downloadLocks.get(file);         if (lock == null) {             Object newLock = new Object();             Object existingLock = downloadLocks.putIfAbsent(file, newLock);             lock = existingLock == null ? newLock : existingLock;         }         try {             synchronized (lock) {                 if (!Files.exists(file)) {                     retrieveResource(file);                 }             }         } finally {             downloadLocks.remove(file, lock);         }     }     serveResource(file); } 

retrieveResource method retrieves resource into a temporary file and then performs atomical move into the final destination, so I assume that it’s safe to use.

My question is if it’s a correct multithreaded code and if there are better suited primitives for this fine-grained synchronization. I’m using Java 7.