Share a single instance of LinkedHashMap in Multithreaded environiment

Question

I need to initialize a single instance of LinkedHashMap while starting my SpringBoot application .I tried to initialize the same in the following way

@Getter
 private Map<Long, String> myMap;

    @Override
    public void setApplicationContext(ApplicationContext applicationContext) {
            synchronized (this) {
                if (null == myMap) {
                    myMap = new LinkedHashMap<>();
                }
            }

    }

My static code analysis tool spotbugs reported the following
EI: May expose internal representation by returning reference to mutable object

I cannot return the copy of LinkedHashMap since it needs to be shared among all the threads. So I decided to supress warning . Is this approach correct or is there any other efficient way?

Answer 1

IMHO you can suppress the warning.

Alternatively, you can do the following:

Instead of providing getter to this instance, you can provide public methods like get or add which will use this Map. It means, threads will not have direct access to your hashmap, but only through the method. In this way, you need not to suppress the warning.

Answer 2

The whole approach is faulty!

You synchronised the creation of the map, but then your getter returns it to the wild, where the access to it is not forced to be synchronised – knowing that it is used by different threads!! Bad idea!

You need to omit the getter and setter; instead you create the map in the constructor and provide methods that modify it – this way you have always full control about the manipulation of the map.

If you need the contents of the map somewhere, you write a 'getter' like this:

public final Map<Long,String> obtainMapContents()
{
  return new LinkedHashMap<>( myMap );

  // Or if you do not need the semantics of LinkedHashMap:
  // return Map.copyOf( myMap );
}

Of course, this may need synchronisation, too. So if you always use the instance as monitor (this), you can write

public synchronized final Map<Long,String> obtainMapContents()
{
  return new LinkedHashMap<>( myMap );

  // Or if you do not need the semantics of LinkedHashMap:
  // return Map.copyOf( myMap );
}

but this has other drawbacks.

The most sophisticated solution would use ReentrantReadWriteLock instances and looks like this:

public final class MyClassWithAMap
{
  private final Map<Long,String> m_Map = new LinkedHashMap<>();
  private final ReadLock m_ReadLock;
  private final WriteLock m_WriteLock;

  public MyClassWithAMap()
  {
    final var lock = new ReentrantReadWriteLock();
    m_ReadLock = lock.readLock();
    m_WriteLock = lock.writeLock();
  }

  public final void addEntry( final Long long, final String string )
  {
    m_WriteLock.lock();
    try
    {
      m_Map.put( long, string );
      m_WriteLock.unlock();
    }
  }

  public final String getEntry( final Long long )
  {
    String retValue = null;
    m_ReadLock.lock();
    try
    {
      retValue = m_Map.get( long );
      m_ReadLock.unlock();
    }
    return retValue;
  }

  public final Map<Long,String> getEntries()
  {
    final Map<Long,String> retValue;
    m_ReadLock.lock();
    try
    {
      retValue = new LinkedHashMap<>( m_Map );
      m_ReadLock.unlock();
    }
    return retValue;
  }

  public final String removeEntry( final Long long )
  {
    String retValue = null;
    m_WriteLock.lock();
    try
    {
      retValue = m_Map.delete( long );
      m_WriteLock.unlock();
    }
    return retValue;
  }
}

but it also might be overkill for your use case.

Answer 3

You can use a wrapper over the map instead of returning the map directly. This way you wont expose the map.

Map<String, String> test = new HashMap<>();

public String get(String key){
    return test.get(key);
}

(just a example test code w/o null or any other checks)