How to make this function thread safe?

Question

public class Sol {



    static Map<Integer, List<String>> emap;
    static List<Integer> sortSalaries(List<List<String>> workers) {
        List<Integer> res = new ArrayList<Integer>();
        emap = new HashMap<>();
        for (List<String> e: workers)
            emap.put(Integer.parseInt(e.get(0)), e);

        for(List<String> worker: workers )
        {
         //accessing workers
         .....
        }


        Collections.sort(res);

        return res;


    }

    public static int dfs(int eid) {
        List<String> employee = emap.get(eid);
        int salary=0;
        String ans = employee.get(3);
        for (int i=0;i<ans.length();i=i+2)

        {
            // accesing emap
          ......
        }

        return salary;
    }


}

Do i have to use synchronized keyword to make it thread safe. Do i have to use Vector and Hashtable if method is synchronized.

Alternatively, What if i use Vector and Hashtable, move the emap variable to sortSalaries() and pass it to dfs(). Is it okay if i not use synchronized keyword in this case..

Answer 1

1. Protect emap from outer access
2. Init emap to exclude NPE

Example:

public final class Sol {

    private static final Map<Integer, List<String>> emap = new HashMap<>();

    static List<Integer> sortSalaries(List<List<String>> workers) {
        synchronized (Foo.class) {
            for (List<String> e : workers)
                emap.put(Integer.parseInt(e.get(0)), e);
        }

        // do smth, not access emap
    }

    public static synchronized int dfs(int eid) {
        // do smth with accessing emap
    }
}

In sortSalaries you can minimize synchoronized block with for loop. In dfs you access emap in different places of the method and therefore you have to synchoonized enire method.

Using either ConcurrentHashMap or Vector do not help here, becuase betwee get/set elements to the collection, they could be changed, which is not OK for dfs method: it should feeze emap when it's called.

Answer 2

I asked you question in comment that - do you understand why these methods are not thread-safe if called from multiple threads? and you pointed me to a link without specifying that if you really understood it or not and why do you think that your class is not thread safe so I am providing a little bit of background instead of directly answering the question.

A Bit of Short Discussion

Any class or its methods might become not thread safe when you start sharing data among runner / calling threads. Your class by default is thread - safe if no data is shared among threads so easiest way to make your class thread - safe is to stop sharing data among threads and in your case, its going to be removal of - emap ( because its a class state and used in methods ) & List<List<String>> workers ( This is what I am not sure of since its a reference passed on from caller and different method calls will be working on same instance or might be different instances are passed to this method ) and replace these by method local variables.

Method local variables are thread - safe by default since new instances are created and destroyed for each call.

if you can't do that or not feasible , follow oleg.cherednik's answer to synchronize for variable - emap - either at block level or method level. Do remember that there are various ways to synchronize in Java with synchronized keyword being easiest.

Now for method parameters - List<List<String>> workers & int eid , synchronization for eid is not needed since you are simply reading it and not updating & also its not pass by reference but pass by value due to type being primitive.

Synchronization for access to List<List<String>> workers is needed if you are passing same list instance to calls of this method from different threads. Refer to Gray's Answer - Here and this point is missed in oleg.cherednik's answer. You are better judge if synchronization would be needed or not for this reference.

Its easy to assume that List iteration is thread- safe ( since you are not updating the list ) but that might not always be true . Refer this question and all answers for detailed discussion.

So summary is this - you start implementing thread - safety for your class by first analyzing if some objects are shared among threads or not. If objects are shared , read / write to those objects need to be synchronized ( to make it atomic & provided those objects are not already thread - safe ) . If no objects are shared - its already thread safe . Also, try to create your classes with already thread - safe data structures , that way you will have less work to do.

java.lang.NullPointerException ( NPE ) point of oleg.cherednik's answer stands too.