HashMap cache sync

Question

I have a web application in which people request resources. These resources are cached using a synchronized hash map to increase efficiency. The problem here is that at the same time there are two different requests for the same unpacked resource: the operation receiving the resources takes up a lot of memory, so I want to avoid calling more than once for the same resource.

Can someone please tell me if there is a potential problem with the following snippet? Thanks in advance.

private Map<String, Resource> resources = Collections.synchronizedMap(new HashMap<String, Resource>()); public void request(String name) { Resource resource = resources.get(name); if (resource == null) { synchronized(this) { if (resources.get(name) == null) { resource = veryCostlyOperation(name); // This should only be invoked once per resource... resources.put(resource); } else { resource = resources.get(name); } } } ... } 

Answer 1

One possible problem is that you create an unnecessary statement by executing veryCostlyOperation() inside a synchronized block, so that many threads cannot simultaneously extract their (independent) resources. This can be solved using Future<Resource> as display values:

 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 

Answer 2

The only potential problem I see is syncing with this . If any other code in the same class also synchronizes with this , only one of these blocks will run immediately. Maybe there is nothing else that does this, and it is wonderful. I am always worried about what the next programmer will do. (or I three months later when I forgot about this code)

I would recommend creating a shared synchronization object and then synchronizing with it.

  private final Object resourceCreationSynchObject = new Object ();

then

  synchronized (this.resourceCreationSynchObject) {
   ...
 }

Otherwise, it does exactly what you are asking for. This ensures that veryCostlyOperation cannot be called in parallel.

It was also great to think about reloading the resource a second time in the synchronized block. This is necessary, and the first call from the outside ensures that you do not synchronize when the resource is already available. But there is no reason to call him a third time. First of all, in the synchronized block, set resource to resources.get(name) again, and then check this variable for null. This will prevent you from calling get again inside the else clause.

Answer 3

Your code looks fine, except that you are synchronizing more than necessary:

• Using ConcurrentHashMap instead of a synchronized HashMap allows multiple calls to the get method without blocking.

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

Answer 4

Your code will potentially call veryCostlyOperation (name) several times. The problem is that after viewing the map there is an unsynchronized step:

 public void request(String name) { Resource resource = resources.get(name); if (resource == null) { synchronized(this) { //... } } //... } 

The get () function from the map is synchronized across the map, but checking the result for null is not protected by anything. If several threads enter this request with the same "name", they will all see a zero result from resources.get () until one of them completes an expensive operation and puts the resource on the resource map.

A simpler and more working, but less scalable approach should be to go with a normal map and make the entire request method synchronized. If in practice, in practice, there is no problem, I would choose a simple approach.

For higher scalability, you can fix your code by checking the card again after synchronization (this) to catch the case described above. It still would not provide better scalability, since synchronized (this) allows only one thread to perform expensive work, while in many practical cases you want to prevent multiple executions for the same resource, while allowing simultaneous requests to different resources. In this case, you will need some means to synchronize on the requested resource. A very simple example:

 private static class ResourceEntry { public Resource resource; } private Map<String, ResourceEntry> resources = new HashMap<String, ResourceEntry>(); public Resource request(String name) { ResourceEntry entry; synchronized (resources) { entry = resources.get(name); if (entry == null) { // if no entry exists, allocate one and add it to map entry = new ResourceEntry(); resources.put(name, entry); } } // at this point we have a ResourceEntry, but it *may* be no loaded yet synchronized (entry) { Resource resource = entry.resource; if (resource == null) { // must create the resource resource = costlyOperation(name); entry.resource = resource; } return resource; } } 

This is just a rough sketch. Basically, it performs a synchronized ResourceEntry search and then synchronizes with a ResourceEntry to ensure that a specific resource will be created only once.