1

First, I had the following (here simplifed) class:

public class MyClass {

    private static Map<String, Object> objects = new HashMap<String, Object>();

    public static Object get(String key) {
        return objects.get(key);
    }

    public static void set(String key, Object object) {
        objects.put(key, object);
    }

}

Then, I wanted to make it treahsafe, so I tried the synchronized keyword as follow:

public class MyClass {

    private static Map<String, Object> objects = new HashMap<String, Object>();

    public static synchronized Object get(String key) {
        return objects.get(key);
    }

    public static synchronized void set(String key, Object object) {
        objects.put(key, object);
    }

}

The question is, is the synchronized keyword sufficient in my case, or is it necessary to add the volatile one, i.e.:

public class MyClass {

    private static volatile Map<String, Object> objects = new HashMap<String, Object>();

    public static synchronized Object get(String key) {
        return objects.get(key);
    }

    public static synchronized void set(String key, Object object) {
        objects.put(key, object);
    }

}

?

sp00m
  • 47,968
  • 31
  • 142
  • 252
  • Everybody's recommending a concurrent hashmap, which will be threadsafe, but the documentation mentions a number of caveats about synchronization, which you may still need to synchronize to avoid. Also, if you know the desired type of the stored object, you should be able to use 'generic' methods to be able to get an 'automatically cast' object (ie, the retrieval method does the casting, not the calling code). – Clockwork-Muse Jan 25 '13 at 17:14

3 Answers3

4

Making objects volatile will only have an impact if you reassign objects. In your example you don't so it won't make a difference and is unnecessary.

Note that it is good practice to enforce "non-reassignability" by making objects final.

In your case, you could simply delegate thread safety by using a thread safe map implementation (which would certainly scale better than your synchronized implementation).

assylias
  • 321,522
  • 82
  • 660
  • 783
  • So you mean I should remove the `synchronized` keyword, and use `private final static Map objects = new ConcurrentHashMap();` instead, right? – sp00m Jan 25 '13 at 16:35
  • @sp00m yes that would work (i.e. it would be thread safe too). – assylias Jan 25 '13 at 16:35
3

volatile does not magically make your code thread safe, it's only here to prevent the JVM from doing optimizations that could not be relevant in a multithreading context (or worse, prevent your program from running as expected).

I suggest you take a look at ConcurrentHashMap instead, it could be useful in your case.

Bastien Jansen
  • 8,756
  • 2
  • 35
  • 53
  • So you mean I should remove the `synchronized` keyword, and use `private final static Map objects = new ConcurrentHashMap();` instead, right? – sp00m Jan 25 '13 at 16:35
  • That's the idea, yes. Please take a look at this thread for further info: http://stackoverflow.com/a/4702138/500478 – Bastien Jansen Jan 25 '13 at 16:36
0

You're not changing the reference that's stored in objects, so there's no value in using the volatile keyword.

Ted Hopp
  • 232,168
  • 48
  • 399
  • 521