Synchronized and local copies of variables

Question

I'm looking at some legacy code which has the following idiom:

Map<String, Boolean> myMap = someGlobalInstance.getMap();
synchronized (myMap) {
    item = myMap.get(myKey);
}

The warning I get from Intelli-J's code inspections is:

Synchronization on local variable 'myMap'

Is this the appropriate synchronization and why?

Map<String, Boolean> myMap = someGlobalInstance.getMap();
synchronized (someGlobalInstance.getMap()) {
    item = myMap.get(myKey);
}

Answer 1

I think it would be better to use synchronized wrapper for your map

Answer 2

I think the code could be sound, depending on what the getMap() method does. If it keeps a reference to an instance that has to be shared between threads it makes sense. The warning is irrelevant since the local variable is not initialized locally.

Answer 3

Alex is correct in that adding a synchronized wrapper by calling Collections.synchronizedMap(Map) is a typical approach here. However, if you take this approach there may still be situations where you need to synchronized on the Map's lock; e.g. when iterating over the map.

Map<String, String> syncMap = Collections.synchronizedMap(new HashMap<String, String>());

// Synchronized on map to prevent ConcurrentModificationException whilst iterating.
synchronized (syncMap) {
  for (Map.Entry<String, String> entry : syncMap.entrySet()) {
    // Do work
  }
}

In your example, the warning from IDEA can be ignored, as it's evident that your local variable: map is retrieved from somewhere else (someGlobalInstance) rather than being created within the method, and can therefore potentially be accessed from other threads.

Answer 4

The reason this is flagged as a problem is because synchronizing on local variables is usually a bad idea.

If the object returned by someGlobalInstance.getMap() is always the same, then the synchronized block does in fact use that quasi-global objects monitor and the code produces the expected result.

I also agree with the suggestion to use a synchronized wrapper, if you only need to synchronize the get()/put() calls and don't have any bigger synchronized blocks. But make sure that the Map is only accessed via the wrapper or you'll have another chance for bugs.

Also note that if someGlobalInstance.getMap() does not return the same object all the time, then even your second code example will not work correctly, it could even be worse than your original code since you could synchronize on a different object than the one you call get() on.