CODEWITHSUNDEEP
javamultithreading
Leem.fin
Leem.fin

Reputation: 42582

Is my code thread safe already

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.

Upvotes: 2

Views: 120

Answers (4)

Ravindra babu
Ravindra babu

Reputation: 38910

  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

Upvotes: 0

MD. Sahib Bin Mahboob
MD. Sahib Bin Mahboob

Reputation: 20514

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();
  }
}

Upvotes: 2

Thomas
Thomas

Reputation: 88707

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.

Upvotes: 4

Preston Garno
Preston Garno

Reputation: 1215

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.

Upvotes: 0

Related Questions