Java visibility: final static non-threadsafe collection changes after construction

Question

I found the following code snippet in luaj and I started to doubt that if there is a possibility that changes made to the Map after it has been constructed might not be visible to other threads since there is no synchronization in place.

I know that since the Map is declared final, its initialized values after construction is visible to other threads, but what about changes that happen after that.

Some might also realize that this class is so not thread-safe that calling coerce in a multi-threaded environment might even cause infinite loop in the HashMap, but my question is not about that.

public class CoerceJavaToLua {
    static final Map COERCIONS = new HashMap(); // this map is visible to all threads after construction, since its final

    public static LuaValue coerce(Object paramObject) {
        ...;
        if (localCoercion == null) {
            localCoercion = ...;
            COERCIONS.put(localClass, localCoercion); // visible?
        }
        return ...;
    }

    ...
}

I don't really understand the question. The `HashMap` is a complex type which means it's an object on the heap. As long as the visibilty isn't declared `private` or `protected` other classes can access the variable and therefore the object. The `final` modifier only ensures that you can't *assign a new object* to the class member `COERCIONS`. However, it doesn't keep you from adding and removing objects from the `Map`. What exactly is your question? — Felix, Aug 25 '15 at 03:08
Its static, means that it will be initialized after the class loader loads it (usually when the VM start the application). And its final, means no thread can reassign the COERCIONS to points to other instance after its initialized. The null check will always false, thus the COERCIONS.put will never get executed. The line is not visible. — Ferdinand Neman, Aug 25 '15 at 03:10
The OP is talking about the visibility aspects of the Java Memory Model. Appallingly few Java developers are even aware that this is an issue. — Kevin Krumwiede, Aug 25 '15 at 03:42

score 3 · Accepted Answer · answered Aug 25 '15 at 03:39

3

You're correct that changes to the Map may not be visible to other threads. Every method that accesses COERCIONS (both reading and writing) should be synchronized on the same object. Alternatively, if you never need sequences of accesses to be atomic, you could use a synchronized collection.

(BTW, why are you using raw types?)

answered Aug 25 '15 at 03:39

Kevin Krumwiede

9,868
4
34
82

That's not OP's code, that's luaj source code he's inspecting. I added the grepcode link to the question to clarify this. – Tagir Valeev Aug 25 '15 at 03:44
1

I copied the source code from luaj. I also had the same question when I first saw it. The reason they did that, I guess, is because the library also wants to support JME and, for some reasons, it needs to be compiled in Java 1.2/1.3. – Kin Cheung Aug 25 '15 at 04:11

score 2 · Answer 2 · answered Aug 25 '15 at 03:50

This code is actually bad and may cause many problems (probably not infinite loop, that's more common with TreeMap, with HashMap it's more likely to get the silent data loss due to overwrite or probably some random exception). And you're right, it's not guaranteed that the changes made in one thread will be visible by another one.

Here the problem may look not very big as this Map is used for caching purposes, thus silent overwrites or visibility lag doesn't lead to real problems (just two distinct instances of coersion will be used for the same class, which is probably ok in this case). However it's still possible that such code will break your program. If you like, you can submit a patch to LuaJ team.

the famous infinite loop case in HashMap [link](http://mailinator.blogspot.com/2009/06/beautiful-race-condition.html) — Kin Cheung, Aug 26 '15 at 09:19
@KinCheung, in modern JDK the implementation differs drastically. Well probably an infinite loop is still possible though, I cannot say for sure. — Tagir Valeev, Aug 26 '15 at 09:27

score 0 · Answer 3 · answered Aug 25 '15 at 03:47

Two options:

// Synchronized (since Java 1.2)
static final Map COERCIONS = Collections.synchronizedMap(new HashMap());

// Concurrent (since Java 5)
static final Map COERCIONS = new ConcurrentHashMap();

They each have their pros and cons.

ConcurrentHashMap pro is no locking. Con is that operations are not atomic, e.g. an Iterator in one thread and a call to putAll in another will allow iterator to see some of the values added.

Java visibility: final static non-threadsafe collection changes after construction

3 Answers3