trjhtr

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.

The handling of Object lock in serveFile looks rather messy: its instantiation or retrieval could be done with just a single call of putIfAbsent.

As @SharonBenAsher already mentioned in his answer, Java 7 has a number of interesting tools to handle concurrency in a more flexible way.

For this case, I'd suggest to use a write lock when the resource is retrieved and a read lock when it is served.

private final ConcurrentMap<Path, ReadWriteLock> downloadLocks = new ConcurrentHashMap<>();

// the original method can be shortened to this:
void serveFile(Path file) 
 if (!Files.exists(file)) 
 retrieveResourceSafely(file);
 
 serveResourceSafely(file);


private ReadWriteLock getLockFor(Path file) 
 if (downloadLocks.containsKey(file)) 
 return downloadLocks.get(file);
 
 final ReadWriteLock lock = new ReentrantReadWriteLock();
 downloadLocks.put(file, lock);
 return lock;


/* 
 Serve the file safely using a _read_ lock. According to the Javadoc on ReadWriteLock,
 "only a single thread at a time (a <em>writer</em> thread)
 can modify the shared data, in many cases any
 number of threads can concurrently read the data 
 (hence <em>reader</em> threads)".
*/
private void serveResourceSafely(Path file) 
 final ReadWriteLock fileLock = getLockFor(file);
 try 
 fileLock.readLock().lock();
 serveResource(file);
 finally 
 fileLock.readLock().unlock();
 


// Retrieves the requested resource with a _write_ lock.
private void retrieveResourceSafely(Path file) 
 final ReadWriteLock fileLock = getLockFor(file);
 try 
 fileLock.writeLock().lock();
 if (!Files.exists(file)) 
 retrieveResource(file);
 
 finally 
 fileLock.writeLock().unlock();
 

The drawback of this approach that I already see is that downloadLocks map will be progressively filled with references to all the resources that were accessed. Emptying this map from time to time would be a solution, but I don't have an idea of the logic of resources handling in your cache.

Java's low level thread synchronization mechanism (synchronized blocks, volatile variables, wait(), notify(), etc) were proven to be tricky for the human single threaded mindset. Threading hazards like deadlock, thread starvation, and race conditions, which result from incorrect use of low level thread synchronization, are also hard to detect and debug.

To that end, Java 7 introduced several new features/classes in the java.util.concurrency package that allow developers to approach the thread synchronization domain from a high(er) level. java.util.concurrent.locks.ReentrantLock is more flexible and gives more fine grain control over the scope of locking. it also performs better. The advice that I see around the web (including on Stack Overflow) is that ReentrantLock should replace synchronized block in all new code.