Is access by reference to a lazily initialized non-volatile String thread-safe?

Question

I have a String field that is initialized to null but then accessed by potentially more than one thread. The value will be lazily initialized to an idempotently calculated value upon first access.

Does this field need to be volatile to be thread safe?

Here's an example.

public class Foo {
    private final String source;
    private String BAR = null;

    public Foo(String source) {
        this.source = source;
    }

    private final String getBar() {
        String bar = this.BAR;
        if (bar == null) {
            bar = calculateHashDigest(source); // e.g. an sha256 hash
            this.BAR = bar;
        }
        return bar;
    }

    public static void main(String[] args) {
        Foo foo = new Foo("Hello World!");
        new Thread(() -> System.out.println(foo.getBar())).start();
        new Thread(() -> System.out.println(foo.getBar())).start();
    }
}

I used System.out.println() for the example but I'm not worried about what happens when its calls are interlocked. (Though I'm pretty sure that's thread-safe too.)

Does BAR need to be volatile?

I'm thinking the answer is No, volatile is not required, and Yes it's thread safe, primarily because of this excerpt from JLS 17.5:

final fields also allow programmers to implement thread-safe immutable objects without synchronization. A thread-safe immutable object is seen as immutable by all threads, even if a data race is used to pass references to the immutable object between threads.

And since the char value[] field of String is indeed final.

(int hash isn't final but it's lazy initialization looks sound as well.)

Edit: Edit to clarify the value intended for BAR is a fixed value. Its calculation is idempotent and has no side-effects. I don't mind if the calculation is repeated across threads, or if BAR becomes effectively a thread-local due to memory-caching / visibility. My concern is, if it's non-null then it's value is complete and not somehow partial.

Answer 1

Your code is (technically) not thread-safe.

It is true that String is a correctly implemented immutable type, and the things that you say about its final fields are correct. But that's not where the thread-safety issues are.

The first issue is that there is a race condition in the lazy initialization of BAR. If two threads call getBar() simultaneously, they will both see BAR as null and both then try to initialize it.

The second issue is that there is a memory hazard. Since there are no happens-before relationships between one thread's write to BAR and another thread's subsequent read of BAR, there is no guarantee that the second thread will see the initialized value of BAR. Hence, it may repeat the initialization.

Note that in the example as written, these two issues are not a practical thread-safety problem. The initialization you are performing is idempotent. It makes no difference to the behavior of the code that you might initialize BAR multiple times, since you always initialize it to a reference to the same String object. (The cost of a single redundant initialization too small to worry about.)

However, if BAR was a reference to a mutable object, or if the initialization was expensive, then this is a real thread-safety problem.

As @Ravindra says, the simple solution is to declare getBar to be synchronized. This addresses both issues.

Your idea of declaring BAR addresses the memory hazard, but not the race condition.

---

You added the following to your Question:

Edit to clarify the value intended for BAR is a fixed value. Its calculation is idempotent and has no side-effects. I don't mind if the calculation is repeated across threads, or if BAR becomes effectively a thread-local due to memory-caching / visibility. My concern is, if it's non-null then it's value is complete and not somehow partial.

This changes nothing that I said above. If the value is a String, then it is a properly implemented immutable object, and you will always see a complete value irrespective of anything else. That's what the JLS quote says!

^{(Actually, I am glossing over the detail that String uses a non-final field to hold a lazily computed hashcode. However, the String::hashCode implementation takes care of that. There is no thread-safety issue with it. Check it for yourself if you like.)}

Answer 2

Your code is not thread safe. It appears you might be thinking of a double-checked locking pattern. The correct pattern goes something like this:

public class Foo {

    private static volatile String BAR = null;

    private static String getBar() {
        String bar = BAR;
        if (bar == null) {
          synchronized( Foo.class )
            if( bar == null ) {
              bar = "Hello World!";
              BAR = bar;
            }
        }
        return bar;
    }
    // ...

So two things here.

1. If BAR is already initialized, the synchronized block isn't entered. volatile is necessary here because some synchronization is needed and the read of BAR will be synchronized with the write to the volatile BAR.

2. If BAR is null then we enter the synchronized block, we have to check again that BAR is still null so we can do the check and the assignment atomically. If we don't check atomically then there's a chance that BAR will be initialized more than once.

You quoted the Java spec. about the final keyword. While String is immutable and uses the final keyword internally, that doesn't affect your field BAR. The string is fine, but your field is still a shared memory location and needs to be synchronized if you expect it to be thread safe.

Also another poster mentioned interning strings. They are correct to say that in this particular instance, there will be only one "Hello World!" object because the JVM spec guarantees that strings are interned. That's a weird form of thread safety that doesn't work for other objects, so only use it when you are sure it will work correctly. Most objects that you create yourself won't be able to use the code you have as it is now.

Lastly I thought I'd point out that because "Hello World!" is a string object already, there's not much point in trying to "lazy load" it. Strings are created by the JVM when your class is loaded, so they already exist by the time your method is run, or even by the time BAR is read for the first time. In this case with just a string there's not advantage to trying to "lazy load" the string.

public class Foo {

    //  probably better, simpler
    private static final String BAR = "Hello World!";