CODEWITHSUNDEEP
javamultithreadingsynchronizationthread-safety
munyengm
munyengm

Reputation: 15479

Is the Map referenced in this class safely published?

Will threads that call IsItThreadsafe.getValue(...) get an up-to-date value from the Map referenced by the variable IsItThreadsafe.map (assuming the exact usage scenario implemented in IsItThreadSafe.main(...)? 

public class IsItThreadSafe {

    private Map<String, String> map; 

    public synchronized void setMap(Map<String, String> map) {
        this.map = map;
    }

    public synchronized String getValue(String key) {
        if (this.map == null) {
            this.map = new HashMap<>();
        }
        return map.get(key);
    }


    public static void main(String[] args) throws Exception {
        IsItThreadSafe sharedObject = new IsItThreadSafe();
        Thread t1 = new Thread(() -> {
            while (true) {
                for (int i = 0; i < 10; i++) {
                    String key = String.valueOf(i);
                    String value = sharedObject.getValue(key);

                    if (value!=null) {
                        System.out.println("key = " + key + " value = " + value);
                    }
                }
            }
        });
        t1.start();

        Thread t2 = new Thread(() -> {
            while (true) {
                Map<String, String> mappedData = new HashMap<>();

                for (int i = 0; i < 10; i++) {
                    mappedData.put(String.valueOf(i), String.valueOf(i + 1));
                }

                sharedObject.setMap(mappedData);
            }
        });
        t2.start();

        t1.join();
        t2.join();
    }
}

Upvotes: 0

Views: 52

Answers (1)

Tagir Valeev
Tagir Valeev

Reputation: 100239

Seems that it should work correctly unless the setMap caller continues to modify the map it passed to setMap. It should forget about this map once the setMap is called. Such version would be more safe in general:

public synchronized void setMap(Map<String, String> map) {
    this.map = new HashMap<>(map);
}

Of course though your code is correct, it doesn't mean that the code is optimal. In particular if you are not going to put new values into the map, it's possible to get rid of synchronization every time you query the value. For example, you may use AtomicReference:

private final AtomicReference<Map<String, String>> map = new AtomicReference<>(); 

public void setMap(Map<String, String> map) {
    this.map.set(new HashMap<>(map));
}

public String getValue(String key) {
    Map<String, String> m = this.map.get();
    // loop is necessary if setMap(null) call occurs
    while (m == null) {
        // Use CAS to avoid possible overwrite of concurrent setMap() call
        m = new HashMap<>();
        if(!this.map.compareAndSet(null, m))
            m = this.map.get();
    }
    return m.get(key);
}

Here synchronization is replaced with volatile read which is in general much faster.

Upvotes: 1

Related Questions