Java Thread safe code

Question

I saw the code below on the web, and it says "the code is not thread safe". I could not understand why? Since, each thread below runs the getList will not let any other one to reach to the getList().

public class MyClass {
    private List<String> list;

    public static void main (String[] args) throws InterruptedException {
        MyClass obj = new MyClass();

        Thread thread1 = new Thread(() -> {
            System.out.println("thread1 : " + System.identityHashCode(obj.getList()));
        });
        Thread thread2 = new Thread(() -> {
            System.out.println("thread2 : " + System.identityHashCode(obj.getList()));
        });

        thread1.start();
        thread2.start();
    }

    private List<String> getList () {
        if (list == null) {
            list = new ArrayList<>();
        }
        return list;
    }
}

Possible duplicate of [How to implement thread-safe lazy initialization?](https://stackoverflow.com/questions/8297705/how-to-implement-thread-safe-lazy-initialization) (ok perhaps not an exact dupe, because it doesn't explain why this way is not safe, it just discusses solutions) — Hulk, May 16 '18 at 06:54

score 6 · Answer 1 · answered May 15 '18 at 11:43

6

This line makes the program not thread-safe

if (list == null) {

Both threads might see the list is null at the same time and try to assign them new array.

answered May 15 '18 at 11:43

Mạnh Quyết Nguyễn

17,677
1
23
51

1- What do you mean by "at the same time"? I thought, once a thread got into it, it wont let others to get in the code section until it finishes? 2- If I change the method to private List getList () { } return list; } Do I still need syncronized for the list? – James May 15 '18 at 11:57
_once a thread got into it, it wont let others to get in the code section until it finishes_ -> This is what `thread synchronization` used for. You have no synchronization so both may access the resource `list` at the sametime – Mạnh Quyết Nguyễn May 15 '18 at 11:58
The probability that "both threads might see the list is null at the *same time*" is actually pretty much zero, that's not the problem. The problem is that [Java memory model](https://en.wikipedia.org/wiki/Java_memory_model) does not guarantee that the thread that arrives at the null-check first will actually share the updated state with the second thread. You have to synchronize to enforce it. – Andrey Tyukin May 15 '18 at 12:05

Andrey Tyukin · Answer 2 · 2018-05-15T11:53:16.760

3

You have mutable state (the member variable list) that is shared between the two threads.

The two threads can access:

if (list == null) {

and modify:

        list = new ArrayList<>();

the shared mutable state through the getlist method, which is not synchronized in any way.

Therefore, this code is not thread safe. Accessing and modifying shared mutable state without synchronization is a bad idea in general.

edited May 15 '18 at 11:53

answered May 15 '18 at 11:47

Andrey Tyukin

43,673
4
57
93

Stephen C · Answer 3 · 2018-05-15T13:15:08.100

This code is unsafe for two reasons, both centering on these statements:

if (list == null) {
    list = new ArrayList<>();
}
return list;

First problem is that there is a race condition. On a system with multiple cores, there is a tiny probability that two threads will read list at exactly the same time, both threads will see null, and two ArrayList objects will be created and returned. And on a single core system, there is an even smaller probability that one thread will be preempted by the other immediately after reading list and you will get the same result.

But there is a more insidious problem that related to the Java Memory Model. The fact that there is no synchronization means that there is no happens-before relationship between one thread writing list and another thread (subsequently) reading it. That means that the Java interpreter / JIT compiler are not obliged to insert memory barrier sequences to ensure that the value written by the earlier thread is visible to the later thread. So even in the case where the two threads run at different times ... the later thread may not see the non-null value written by the earlier thread due to various memory caching effects.

The solution to both problems is to synchronize properly; e.g. like this:

synchronized (this) {
    if (list == null) {
        list = new ArrayList<>();
    }
    return list;
}

or by declaring the method to be synchronized.

Java Thread safe code

3 Answers3