DCL with two synchronized block is broken?

Question

I could not understand below code snippet from A fix that doesn't work. (I did read the explanation that follows on same page).

If we have 2 synchronized blocks, how is this DCL version broken? Or is it not applicable post Java5?

// (Still) Broken multithreaded version
// "Double-Checked Locking" idiom
class Foo { 
  private Helper helper = null;
  public Helper getHelper() {
    if (helper == null) {
      Helper h;
      synchronized(this) {
        h = helper;
        if (h == null) 
            synchronized (this) {
              h = new Helper();
            } // release inner synchronization lock
        helper = h;
        } 
      }    
    return helper;
    }
  // other functions and members...
  }

The inner `synchronized` block is completely meaningless. The thread does _not_ acquire the lock when it enters the inner block because it already owns it, and it does not release the lock when it leaves the inner block because it must continue to own it until it leaves the outer block. — Solomon Slow, Nov 20 '15 at 15:09

David Schwartz · Accepted Answer · 2015-11-20T08:10:30.070

3

There's no guarantee that a thread that sees helper as not null will be able to see all the writes made by new Helper();. So you could access a corrupt version of the singleton. You need something in a thread that sees helper as non-null to guarantee that it sees that after the h = new Helper(); completes. Observing a change to a non-volatile variable doesn't establish such a relationship, and that's all that thread does.

Oversimplifying a bit, the way Java's memory visibility model works is that two threads each do something that establishes a "happens before" / "happens after" relationship between the two operations done by the two threads. That can include operations inside a synchronized block or access to a volatile variable.

But with your code above, a thread can observe that helper is not null and then go on to access the object created by new Helper(). It doesn't have to access a volatile variable, nor does it have to enter a synchronized block. So there is nothing that could possibly establish the required "happens after" relationship to ensure it sees any changes made by new Helper().

edited Nov 20 '15 at 08:10

answered Nov 20 '15 at 08:05

David Schwartz

179,497
17
214
278

Ah, so its similar to normal DCL pattern, we are not avoiding access to incompletely constructed object. So the second synchronized block doesnt serve any purpose. Thanks! – ADJ Nov 20 '15 at 08:12
1

Correct. Though in practice it may change it from "happens to fail sometimes" to "happens to work reliably on some systems that I tried it on". – David Schwartz Nov 20 '15 at 08:13
The synchronized block doesn't serve any purpose for two reasons: 1) Things that occur after the exit of the synchronized block aren't synchronized in any way, and the write to `helper` is after the exit. 2) Even if it did guarantee that the writes occurred in order, that wouldn't guarantee that they would be observed in order because non-volatile, unsynchronized reads can be re-ordered. – David Schwartz Nov 20 '15 at 08:15
So now I've given you two explanations, one in the answer and one in the above comment. Both are correct. But I believe the one in the answer is *much* better. It's better to think about it as "this is not guaranteed to work, end of story" than "here's two ways this can go wrong, so don't rely on it". Even if you couldn't think of a way it could go wrong, if it's explicitly not guaranteed to work, you'd be a fool to rely on it. – David Schwartz Nov 20 '15 at 08:16
On reading the original article again, now I understood the problem. The actual problem is assignment helper=h, could be moved by compiler to be inside the inner synchronized block, which takes us back to original DCL issue (helper=h causes second thread to access possibly incomplete object) – ADJ Nov 20 '15 at 08:24
1

@Tingya That's just one way it can go wrong. It's just not guaranteed to work, period. Even if we couldn't think of any way it could go wrong, it still wouldn't be guaranteed. And here's another way it can go wrong: The unsynchronized, non-volatile reads (of `helper` itself and of its members) could be re-ordered. See my answer for a clear explanation of why it's not guaranteed to work, regardless of whether we can imagine how it might go wrong. – David Schwartz Nov 20 '15 at 08:30

DCL with two synchronized block is broken?

1 Answers1