Visibility effects of synchronization in Java

Question

This article says:

In this noncompliant code example, the Helper class is made immutable by declaring its fields final. The JMM guarantees that immutable objects are fully constructed before they become visible to any other thread. The block synchronization in the getHelper() method guarantees that all threads that can see a non-null value of the helper field will also see the fully initialized Helper object.

public final class Helper {
  private final int n;

  public Helper(int n) {
    this.n = n;
  }

  // Other fields and methods, all fields are final
}

final class Foo {
  private Helper helper = null;

  public Helper getHelper() {
    if (helper == null) {            // First read of helper
      synchronized (this) {
        if (helper == null) {        // Second read of helper
          helper = new Helper(42);
        }
      }
    }

    return helper;                   // Third read of helper
  }
}

However, this code is not guaranteed to succeed on all Java Virtual Machine platforms because there is no happens-before relationship between the first read and third read of helper. Consequently, it is possible for the third read of helper to obtain a stale null value (perhaps because its value was cached or reordered by the compiler), causing the getHelper() method to return a null pointer.

I don't know what to make of it. I can agree that there is no happens before relationship between first and third read, at least no immediate relationship. Isn't there a transitive happens-before relationship in a sense that first read must happen before second, and that second read has to happen before third, therefore first read has to happen before third

Could someone elaborate more proficiently?

Answer 1

It's all explained here https://shipilev.net/blog/2014/safe-public-construction/#_singletons_and_singleton_factories, the issue simple.

... Notice that we do several reads of instance in this code, and at least "read 1" and "read 3" are the reads without any synchronization ... Specification-wise, as mentioned in happens-before consistency rules, a read action can observe the unordered write via the race. This is decided for each read action, regardless what other actions have already read the same location. In our example, that means that even though "read 1" could read non-null instance, the code then moves on to returning it, then it does another racy read, and it can read a null instance, which would be returned!

Answer 2

No, there's no any transitive relationship between those reads. synchornized only guarantees visibility of changes that were made within synchronized blocks of the same lock. In this case all reads do not use the synchronized blocks on the same lock, hence this is flawed and visibility is not guaranteed.

Because there is no locking once the field is initialized, it is critical that the field be declared volatile. This will ensure the visibility.

private volatile Helper helper = null;

Answer 3

No, there is no transitive relationship.

The idea behind the JMM is to define rules that JVM must respect. Providing the JVM follows these rules, they are authorized to reorder and execute code as they want.

In your example, the 2nd read and the 3rd read are not related - no memory barrier introduced by the use of synchronized or volatile for example. Thus, the JVM is allowed to execute it as follow:

 public Helper getHelper() {
    final Helper toReturn = helper;  // "3rd" read, reading null
    if (helper == null) {            // First read of helper
      synchronized (this) {
        if (helper == null) {        // Second read of helper
          helper = new Helper(42);
        }
      }
    }

    return toReturn; // Returning null
  }

Your call would then return a null value. Yet, a singleton value would have been created. However, sub-sequent calls may still get a null value.

As suggested, using a volatile would introduce new memory barrier. Another common solution is to capture the read value and return it.

 public Helper getHelper() {
    Helper singleton = helper;
    if (singleton == null) {
      synchronized (this) {
        singleton = helper;
        if (singleton == null) {
          singleton = new Helper(42);
          helper = singleton;
        }
      }
    }

    return singleton;
  }

As your rely on a local variable, there is nothing to reorder. Everything is happening in the same thread.