2

I have a singleton class:

public class MySingleton {
  private static MySingleton instance;

  // here the Map entry's value is also a Map
  private Map<String, Map> dataMap;

  private MySingleton() {
     dataMap = new HashMap<String, Map>();

     Map<String, String> dataTypeOneMap = new HashMap<String, String>();
     Map<String, String> dataTypeTwoMap = new HashMap<String, String>();

     dataMap.put("dataTypeOne", dataTypeOneMap);
     dataMap.put("dataTypeTwo", dataTypeTwoMap);

  }

  public static MySingleton getInstance() {
     if(instance == null) {
        instance = new MySingleton();
     }
     return instance;    
  }

  public synchronized void storeData(String data, String type) {
       dataMap.get(type).put("my_data", data);
  } 

  public synchronized String getData(type) {
       return dataMap.get(type).get("my_data");
  } 
}

Multiple threads can access the public methods storeData(...) & getData(...). e.g.:

MySingleton.getInstance().storeData("hello", "dataTypeOne");

MySingleton.getInstance().getData("dataTypeOne");

Do I need to use ConcurrentHashMap type for dataMap? or is it already thread safe? I feel my code is already thread safe but just want to make sure no corner case would break it. Thanks.

Leem.fin
  • 40,781
  • 83
  • 202
  • 354
  • http://stackoverflow.com/questions/11165852/java-singleton-and-synchronization – Suraj Rao Mar 10 '17 at 13:04
  • 4
    Does it really have to be lazy? *That* part is not thread safe. – Fildor Mar 10 '17 at 13:04
  • @Fildor, could you please share more details what is in your mind? Thanks. – Leem.fin Mar 10 '17 at 13:05
  • Apart from Question: Mind that you get a NPE in storeData if value of `type` is not in map. – Fildor Mar 10 '17 at 13:06
  • Think of 2 threads calling `getInstance()` for the first time: both could get different instances because at the time of calling `instance` is null and you'd then lose data if one thread calls `storeData()` on an instance that's overwritten by the other. – Thomas Mar 10 '17 at 13:06
  • Not sure about your requirements. But the code above is threadsafe..multiple invocations of storeData thr diff threads will not tamper your HashMap – Jaydeep Rajput Mar 10 '17 at 13:06
  • @Jai No it's not threadsafe. It's a common boobytrap to implement the lazy instantiation not thread safe. So two instances could actually be created! – Fildor Mar 10 '17 at 13:08
  • @Thomas would synchronizing `getInstance` fix that? – Jack Flamp Mar 10 '17 at 13:08
  • http://javarevisited.blogspot.de/2014/05/double-checked-locking-on-singleton-in-java.html – Fildor Mar 10 '17 at 13:10

4 Answers4

4

As per the comments the lazy instantiation of the instance is not threadsafe, i.e. if 2 threads call getInstance() simultaneously both calls could result in instance = new MySingleton(); and even return a different instance which would mean both threads operate on different objects.

To fix that you could synchronize getInstance() on the class itself and even use double-checked locking to reduce synchronization overhead (if you can't remove the lazyness in the first place):

private volatile MySingleton  instance;

public static MySingleton getInstance() {
  //check if the instance doesn't exist yet, this is not threadsafe
  if(instance == null) {
    //the instance doesn't exist yet so synchronize and check again, since another thread 
    // might have entered this block in the meantime and instance is not null anymore
    synchronized(MySingleton.class) {
      if(instance == null) {
        instance = new MySingleton();
      }
    }
  }
  return instance;    
}

Using double-checked locking you'd be able to only synchronize the creation part, checking for null and returning the instance if it isn't null doesn't have to be threadsafe.

Thomas
  • 87,414
  • 12
  • 119
  • 157
  • Does `instance` still has to be volatile in Java 8? – Fildor Mar 10 '17 at 13:13
  • 1
    Complicated stuff :) https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java – Jack Flamp Mar 10 '17 at 13:16
  • 1
    @Fildor ah, I'm not entirely sure but according to [this Oracle doc](https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html) "reads and writes are atomic for reference variables " so I'd say no. - Update: as per the wiki article linked by Jack volatile might still be useful to prevent errors due to reordering by the compiler. And adding `volatile` shouldn't hurt anyways. – Thomas Mar 10 '17 at 13:16
  • @Thomas, so, as a conclusion, there is no need to use ConcurrentHashMap for `dataMap`, right? Only the lazy initialization is a issue to fix? And using `volatile` is nice to have for the `instance` variable, am I all right? – Leem.fin Mar 10 '17 at 13:22
  • 1
    @Leem.fin have a look at the wiki article linked by Jack. I'd include `volatile` to be sure. As for the maps: you'd not need a `ConcurrentHashMap` if you synchronize all methods that access the maps but removing that synchronization and using `ConcurrentHashMap` instead could improve performance, since it uses (mostly) lock-free algorithms (i.e. writes/reads on different buckets often don't need to be synchronized at all). – Thomas Mar 10 '17 at 13:26
  • @Fildor Volatile will ensure the integrity regarding the fact that you always get the actual reference rather than any cache. I will definitely add that. – MD. Sahib Bin Mahboob Mar 10 '17 at 13:26
  • You don't actually need `volatile` in this example to be thread safe, because you have the `synchronized` keyword around all access to the `instance` variable. It will however give marginally better performance in calls to `getInstance()` when first checking `if(instance == null)` to prevent it from entering the inner `synchronized(MySingleton.class)` unnecessarily – Brad Mar 10 '17 at 13:32
  • I prefer the holder pattern anyway but I conclude, I would make it volatile just to be on the safe side ... – Fildor Mar 10 '17 at 13:35
  • @Leem.fin this answer seems legit. you should accept it – Jack Flamp Mar 10 '17 at 13:53
  • Actually, this double-check pattern isn't recommended. See the Brian Goetz Concurrency book, p.348 - the recommended way is to use an inner resource holder class for lazy initialisation. – BarrySW19 Mar 10 '17 at 17:29
2

Short answer:

This portion of your code is not thread-safe:

  public static MySingleton getInstance() {
    if(instance == null) {
      instance = new MySingleton();
    }
    return instance;    
  }

Long answer

Let's tackle the code portions one by one.

For instance, assume that one thread is executing getInstance method and reached to if condition:

if(instance == null) {

Now if another thread starts to call the method, the if condition will still be true and it will also try to create a new instance. So it may create some use cases:

  1. Two calling method will end up with the different instance.
  2. Later thread overrides the instance of the previously created instance or vice versa.

Solution:

Synchronize the instance creation block:

synchronized(MySingleton.class){
  if(instance == null) {
    instance = new MySingleton();
  }
}
MD. Sahib Bin Mahboob
  • 20,246
  • 2
  • 23
  • 45
0

No, it's not thread safe. Use a ConcurrentHashMap to allow access and modification from different threads. It'll also allow you to read from the Map asynchronously without performance hits.

Synchronize the getInstance() method, and minimize the amount of calls to that method - storing a reference to the singleton on each object running on another thread to keep from repeated getInstance() calls will help.

Preston Garno
  • 1,175
  • 10
  • 33
0

  1. Thread safe Singleton

    You can define thread safe LazySingleton by following this post:

    Why is volatile used in this example of double checked locking

  2. Your dataMap is thread safe. But you can further optimize your code. You can replace Map with ConcurrentHashMap and remove synchronized in both storeData and getData methods.

You can have a look at this documentation page regarding high level concurrency

Community
  • 1
  • 1
Ravindra babu
  • 37,698
  • 11
  • 250
  • 211