Concurrency for a class with static methods and initialization method

Question

How do I make the following class threadsafe?

public class Helper
{
    private static Map<String,String> map = null;

    public static init()
    {
        map = new HashMap<String,String>();
        // some loading stuff
    }

    public static getMap()
    {
       if(map==null) init();
       return new HashMap<String,String>(map);
    }
}

My ideas so far:

1. Make getMap() synchronized. Problem: Makes the program slower than necessary, because syncronization is only needed at the start of the program and then never again.

2. Use a lock. The problem is that the method "isLocked" which I use here doesn't exist. So what would be the best solution?

   public class Helper
   {
    private static Map<String,String> map = null;
    private static Lock lock = new ReentrantLock();
   
    public static init()
     {
        lock.lock();
        map = new HashMap<String,String>();
        // some loading stuff
        lock.unlock();
   }
   
   public static getMap()
   {
      synchronized {if(map==null) init();}
      while(lock.isLocked()) {Thread.wait(1);]
      return new HashMap<String,String>(map);
   }
   

   }

P.S.: Sorry for the second source code display problem. There seems to be a bug with using code after an enumeration.

P.P.S.: I know HashMap isn't thread safe. But that only means that I cannot write in parallel, reading shouldn't be a problem, should it?

P.P.P.S.: My final version (just the inner class), following John Vint:

protected static class LazyLoaded
{
    static final Map<String,String> map;
    static
    {
        Map<String,String> mapInit = new HashMap<>();
                    // ...loading...
        map = Collections.unmodifiableMap(mapInit); 
    }
}

Answer 1

Simply synchronizing access to the map reference doesn't solve the problem. After the map is created, you need to synchronize operations which access and possibly modify the map's contents. An exception would be if you knew that the map is filled one time during initialization and afterwards only read operations are performed. In that case you may be fine without explicit synchronization, because your control flow takes care of that (knowing that your program's logic only allows reads after the map was initialized).

Otherwise, I would suggest you use one of the ConcurrentMap implementations, for example ConcurrentHashMap since they are relatively cheap if there is no lock contention and still provide the necessary thread safety if you do perform reads and writes during the map's lifetime.

As for the initialization, since it's a static field (so one instance only) and the cost of creating an empty map one time isn't high, I'd suggest you simply declare your map like so:

private static final Map<String,String> map = new ConcurrentHashMap<String,String>();

This way you don't need the conditional code in other methods, there are no reference visibility issues and the code gets simpler.

Answer 2

I would delegate to a child class

public class Helper
{
    public static Map<String,String> getMap()
    {
         return HelperDelegate.map;
    }

    private static class HelperDelegate { 
           private static Map<String,String> map = new HashMap<String,String>();
           static{
                 //load some stuff
           } 

    }
}

Because class loading is thread-safe and occurs only once this will allow you to lazily initialize your Map.