Java and thread safety when creating a cache from scratch

Question

Is the following code thread safe?

private static HashMap<String, String> cache = null;

public static String getInfo(String key) {
    populateCache();

    return cache.containsKey(key) ? cache.get(key) : null;
}

private static void populateCache() {
    if (cache == null) {
        cache.put("hello", "world");

        // ... more population logic here
    }
}

If not, what can I do to make it thread safe?

EDIT: Thanks for the answers, I'll try out ConcurrentHashMap. However, I missed out something.. I am actually passing an object of a separate Foo class to getInfo() here:

public static Bar getInfo(Foo object) {
    object.someProperty = concurrentHashMapCache.get('something');

    return Bar.createFromFoo(object);
}

As long as different threads pass a different Foo object to getInfo, the above code should work, right?

Have you got a delete method as well? question is no because if multiple threads call the same method you can get race conditions. You can use the keyword synchronize to make it mutually exclusive — Montaldo, Nov 08 '13 at 23:02
That code is not thread safe, and will fail with NullPointerException — Damian Leszczyński - Vash, Nov 08 '13 at 23:02
@Vash the code is not thread safe, two threads can execute them at the same time and the cache would be populated twice. — Luiggi Mendoza, Nov 08 '13 at 23:02
Do not reinvent the wheel, use an already built cache like [ehcache](http://ehcache.org) or [infinispan](http://infinispan.org/). — Luiggi Mendoza, Nov 08 '13 at 23:03
@LuiggiMendoza, That was more about the thing that one thread will call an raise an exception but no joke here ;-). — Damian Leszczyński - Vash, Nov 08 '13 at 23:04
Again, please **do not reinvent the wheel**. There are already tested and proven solutions for cache implementations in Java. I posted two in my last comment. — Luiggi Mendoza, Nov 08 '13 at 23:11
@LuiggiMendoza I don't have the option to use a third party product here. I'll unfortunately have to stick to what comes out of the box. — Sayak Banerjee, Nov 08 '13 at 23:13
Thread safety is a complex topic with some pretty subtle edge cases; SO is really not well suited as a site to learn about it. I highly recommend you teach yourself more comprehensively, for instance by getting a copy of Java Concurrency In Practice. This isn't to say that questions about specific concurrency issues don't belong on SO, but the broadness of "is this thread safe? what can I do to make it be?" suggests that maybe you need to go and learn about concurrency in a more cohesive way. IMHO. — yshavit, Nov 08 '13 at 23:16

score 3 · Accepted Answer · answered Nov 08 '13 at 23:02

3

HashMaps in Java are not thread safe. If you want a thread safe version use ConcurrentHashMap: http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html

answered Nov 08 '13 at 23:02

Dale Myers

2,703
3
26
48

or you can use `HashTables` which are `synchronized` http://docs.oracle.com/javase/7/docs/api/java/util/Hashtable.html – Prateek Nov 08 '13 at 23:04
@LuiggiMendoza Yeah ConcurrentHashMaps are more efficient but I just added the possible choices for what OP wants. – Prateek Nov 08 '13 at 23:08
1

@Prateek it's like offering `Vector` class as an option for a synchronized `List`: http://stackoverflow.com/q/1386275/1065197 – Luiggi Mendoza Nov 08 '13 at 23:10
@LuiggiMendoza Yes, but unless the OP is writing some production code (which I think he is not) it doesn't matter. It is not bad to know all your options both efficient and non-efficient and use one based on your needs. As mentioned in my earlier comment I was adding to the list of options that OP has. – Prateek Nov 08 '13 at 23:12
@Prateek OP's trying to build its own cache, so looks like production code to me. If that's the main problem, then it would be better using an already tested and proven cache solutions instead of writing one from scratch. And IMO you should know they were options in the past but **must not** use them in current applications (at least that you're stuck with Java 1.4.2 or prior and don't have any other option =\\). – Luiggi Mendoza Nov 08 '13 at 23:14
@LuiggiMendoza If it is production code I would also suggest to go with already implemented cache solution rather than writing one yourself. And if it is not, there are different choices that OP has. I would myself choose `ConcurrentHashMap` over `HashTables` but it never hurts to know about all the options if you are trying to learn stuff. – Prateek Nov 08 '13 at 23:19
@Prateek IMO learning about usage of `Hashtable` and `Vector` classes is like learning about poisons by drinking them =\. – Luiggi Mendoza Nov 08 '13 at 23:27
@LuiggiMendoza In that case we have different notions of learning things. I would rather know about stuff and then decide on what I should and what I shouldn't use but you have different take on that. – Prateek Nov 08 '13 at 23:30

score 2 · Answer 2 · answered Nov 08 '13 at 23:02

2

It's not. Either use ConcurrentHashMap or make the public method synchronized (which makes more sense since you've got a race condition also on creating the HashMap).

answered Nov 08 '13 at 23:02

Danstahr

4,190
22
38

Just to add more info to your answer. – Luiggi Mendoza Nov 08 '13 at 23:12
@LuiggiMendoza : Okay, thanks. I always ask myslef "Nah, what have I messed up this time?" when I see a comment like this :). – Danstahr Nov 08 '13 at 23:17

score 0 · Answer 3 · answered Nov 08 '13 at 23:04

Much depends not only on simply how a class is written, but also on how it will be used.

You have a single shared mutable field which makes up the state of your class's objects.

cache is populated without any attempts to synchronize access, so it is possible in a concurrent environment for two client threads to be modifying cache at the same time.

In the absence of any kind of internal syncronization, this class looks unsafe.

Java and thread safety when creating a cache from scratch

3 Answers3