Is this thread safe

Question

Is this thread safe? Do I need the lock?

public class Load {

    //private final Object lock = new Object();

    private final Map<String, String> confs = new HashMap<String, String>();

   public void addValue(String key, String value) {
       //synchronized (lock)
       confs.put(key, value);

   }

   public String getValue(String key) {
      //synchronized (lock)
      return confs.get(key);
   }
}

I think it's thread safe without lock. when I do new Load(), there will be new hashmap instance. is this correct?

It depends how you want to access the Load object (for example if you modify and read only from the event dispatch thread it is ok). Usually this is not thread-safe. — Gábor Bakos, May 16 '14 at 13:07
You should think of ConcurrentHashMap instead of lock in this case. — Mik378, May 16 '14 at 13:07
Not thread safe. You have mutable, shared state that's not guarded in any way. — duffymo, May 16 '14 at 13:08
This is not thread safe .. you shall use ConcurrentHashMap if you want thread safety — Sanjeev, May 16 '14 at 13:10

score 1 · Answer 1 · answered May 16 '14 at 13:10

1

HashMap is not thread-safe by default. If you need thread-safe version, use ConcurrentHashMap or you can work with locks manually.

answered May 16 '14 at 13:10

nikis

11,166
2
35
45

Thanks. but still
when I do new Load(), there will be new hashmap instance. how could two thread come to the map?
– user3644708 May 16 '14 at 13:23
@user3644708 during just constructor call you won't get any problems. But if later you will use `addValue` method of the same `Load` object from different threads, you can get an exception. – nikis May 16 '14 at 13:29

score 0 · Answer 2 · edited May 23 '17 at 11:50

This is not thread safe. If two threads call addValue at the same time it is very likely that your HasMap will become corrupt.

You could fix it by using a ConcurrentHasMap.

See:

Hashmap concurrency issue

and especially

A Beautiful Race Condition

Here's a demonstration of using your Load class that would probably crash horribly.

public class Load {

    private final Map<String, String> confs = new HashMap<String, String>();

    public void addValue(String key, String value) {
        confs.put(key, value);

    }

    public String getValue(String key) {
        //synchronized (lock)
        return confs.get(key);
    }
}

public class Loader implements Runnable {
    private final Load load;

    public Loader (Load load) {
        this.load = load;
    }

    @Override
    public void run() {
        for (int i = 0; i < 100000; i++) {
            load.addValue("" + i, "Value:" + i);
        }
    }

}
public void test() throws InterruptedException {
    System.out.println("Hello");
    Load load = new Load();
    Loader l1 = new Loader(load);
    Loader l2 = new Loader(load);
    Thread t1 = new Thread(l1);
    Thread t2 = new Thread(l2);
    t1.start();
    t2.start();
    t1.join();
    t2.join();
}

Thanks. when I do new Load(), there will be new hashmap instance. how could two thread come to the map? — user3644708, May 16 '14 at 13:26
@user3644708 - If the same `Load` object is being used by two threads. — OldCurmudgeon, May 16 '14 at 13:29
@user3644708 you ask if the code is thread safe and people are giving valid answers: no it is not. You sketch a situation here where there never will be any risk of collision at all. The code is thread safe when it can safely be used by multiple threads at the same time - that is not the case here. If that is not what you want to know, then change your question. — Gimby, May 16 '14 at 13:39

score 0 · Answer 3 · answered May 16 '14 at 13:10

No it is not thread safe. Try using ConcurrentHashMap

public class Load {

    //private final Object lock = new Object();

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

   public void addValue(String key, String value) {
       //synchronized (lock)
       confs.put(key, value);

   }

   public String getValue(String key) {
      //synchronized (lock)
      return confs.get(key);
   }
}

score 0 · Answer 4 · answered May 16 '14 at 13:11

No it is not thread safe. You need to put lock in code. Try using ConcurrentHasMap.

Check this code, if you have used normal HashMap, that would give you an exception:

package com.concretepage;
import java.util.Iterator;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
public class ConcurrentHashMapTest implements Runnable{
  private String name;
  private static Map<String,String> conpage=new ConcurrentHashMap<String,String>();
  ConcurrentHashMapTest(String name){
      conpage.put("1","A");
      conpage.put("2","B");
      conpage.put("3","C");
      this.name=name;
  }
  public void run() {
    try{
      Iterator<String> it = conpage.keySet().iterator();
      while(it.hasNext()){
        String key=it.next();
        conpage.put("A"+key, "A"+key);
      }
      System.out.println(name +" completed.");
    }catch(Exception e){
          e.printStackTrace();
    }finally{
    }
  }
  public static void main(String[] args) {
      ExecutorService  executor= Executors.newCachedThreadPool();
      executor.execute(new ConcurrentHashMapTest("Thread one"));
      executor.execute(new ConcurrentHashMapTest("Thrad two"));
      executor.shutdownNow();
  }
}

Is this thread safe

4 Answers4