If you are correct in that CompassSettings is only used once (by a single thread) and never used again, then you can dispense with synchronization altogether. You ought to null out the reference to ensure it actually can't be used again. Leaving the code the way it is is misleading.

I could make it more bullet proof by adding the same check done before the sync block within the sync block as well.

If multiple threads might access getSettingGroups simultaneously ...

2. Is this code so performance sensitive that you feel there is a need to attempt to not synchronize? Did you actually take measurements to know it is so? I suspect not.

3. You are wrong about the worse case. Say one thread, the first one, makes it into the sychronized block and is part way into creating the inner "groupSettings" Map. The VM/processor/compiler might decide to actually store the reference to "gropuSettings" inside the "groups" map before "groupSettings" is fully populated. Meanwhile, another thread finds this Map in "groups" outside the synchronized block and never needs to enter the synchronized block because it found the corresponding "groupSettings". However, the "groupSettings" isn't finished being initialized. See, the VM/processor/compiler is allowed to do that re-ordering because the the other thread never synchronized on "groups". Shay, even if you don't understand/agree with what I'm saying, please just do #1 above or move the synchronized code around the use of "groups", depending on wether multiple threads will actually hit this code. Thread-safety is a poorly understood problem. It's a pet interest of mine.

As for this issue, it is important for me to implement this correctly. I agree that in terms of performance it won't be that bad. There is a case that I just thought about, where people can plug in custom code into Compass, where they might use this code in a multi threaded scenario. Another reason for wanting to implement this correctly is because this idiom is used in another place in Compass, which is used in a multi threaded env and important to be as concurrent as possible,

As for number 3, I think that what you are saying is that the groups.put(settingPrefix, map) might happen before the map variable is fully initialized (has nothing to do with the groupSettings, since it is not the one you check on before the sync block). I do not believe that the compiler/vm can perform this optimization, because of how to code is structured. Can you double check it again?

Last, Compass now comes with backport util concurrent, so I can use a ConcurrentHashMap for groups and completely remove the synchronized block. What do you think?

ConcurrentHashMap looks like the right choice. There is a lingering concern I have with that, though. I know it works fine in Java 1.5, though I am concerned if it actually works for 1.4. I thought the Java memory model was cleaned up in 1.5... so all the "volatile" assumptions that ConcurrentHashMap makes aren't correct for 1.4 – I think. I'll have to do more digging and maybe ask on the JMM mailing list.