Can this class be thread safe lazy loading, or is it as unsafe as "double checked locking"?

Question

The sample code is as follows

public class Lazy<T> implements Supplier<T> {

    public Lazy(Supplier<T> supplier) {
        this.supplier = Objects.requireNonNull(supplier);
    }

    Supplier<T> supplier;

    T value;

    @Override
    public T get() {
        if (supplier != null) {
            synchronized (this) {
                if (supplier != null) {
                    value = supplier.get();
                    supplier = null;
                }
            }
        }
        return value;
    }
}

I am worried that if "supplier" is a constructor. "supplier=null" may be executed before the object is initialized. An error similar to "double checked locking broken" may happen.

"supplier.get()==null" may be true in this class. So I don't check if the value is null

If it is thread unsafe, Should I add "volatile" before the "supplier" field? If it is thread safe, why?

Isn't it better to use another variable instead of setting supplier=null? — Marc, Jul 09 '20 at 03:04
`get` is an instance method, so the only way to call it is via an instance of `Lazy`, as such you _need_ to call the constructor of `Lazy`, as such why not make `Supplier` `final` and get rid of the `if (supplier != null) {` check? And why do you synchronize of the _result_ of `get` instead of the `supplier` itself? Otherwise `T` must be volatile, yes. — Eugene, Jul 09 '20 at 03:07
Thank you for reminding me. I wrote the wrong sample code. I have corrected the content — user10339780, Jul 09 '20 at 03:13

Eugene · Accepted Answer · 2020-07-09T03:46:18.707

3

This is a little bit complicated, but read this. In short, without volatile, this is broken. The entire construct can be greatly simplified:

public class Lazy<T> implements Supplier<T> {
    
     private final Supplier<T> supplier;

     volatile boolean computed = false;

     T value;

     public Lazy(Supplier<T> supplier) {
          this.supplier = Objects.requireNonNull(supplier);
     }

     @Override
     public T get() {
          if (!computed) {
                synchronized (this) {
                   if (!computed) {
                        value = supplier.get();
                        computed = true;
                   }
                }
           }

        return value;
     }

}

edited Jul 09 '20 at 03:46

answered Jul 09 '20 at 03:14

Eugene

117,005
15
201
306

@user10339780 it is up to _you_ to properly react in code for when that happens. "will not work" in exactly what sense? may be you want to throw an Exception when that happens? I can't read minds – Eugene Jul 09 '20 at 03:21
I hope that the cached results do not need to be calculated repeatedly after the first call to Supplier. If "supplier.get() == null", Supplier.get will be called repeatedly in your code. – user10339780 Jul 09 '20 at 03:28
@user10339780 correct. So you wanna say that you want to cache a `null` value too? Meaning that `null` is perfectly fine to be stored in `value` and computed only once? This would be a rather weird invariant – Eugene Jul 09 '20 at 03:32
I just hope that I can simply use "lazy supplier" without having to judge whether the return value is null – user10339780 Jul 09 '20 at 03:39
@user10339780 I guess that is the reason you introduced that supplier null check, on purpose. The better way would be to introduce another variable, see edit. But I honestly don't know what this gives to the callers, how can they use that `null` now in a meaningful way? – Eugene Jul 09 '20 at 03:46
Two minor corrections: the if statement inside the synchronized block must be on the "computed" variable again. Also, because computed is being set inside a synchronized block, I believe it does not have to be volatile as the synchonized block establishes a "happens-before" relationship on the actions inside the block (see https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html) – Marcio Lucca Jul 09 '20 at 03:50
@MarcioLucca 1) corrected 2) happens-before is established _only_ when the reader and writer synchronize on the same lock : _...all actions subsequent to any thread locking that monitor_ (that meaning same monitor). So you do need `volatile` here. – Eugene Jul 09 '20 at 04:01
But we are synchronizing on the same lock/monitor: the "this" object. – Marcio Lucca Jul 10 '20 at 03:28
@MarcioLucca if this would have been the case, why would the `DCL` require `volatile` then ( otherwise it is broken )? Is the code doing a read of `computed` _outside_ the `synchronized` block may be, once? – Eugene Jul 10 '20 at 10:47
@Eugene: are you sure DCL requires volatile on primitive types like boolean (in this case)? If it does, then I claim the following sentence in the javadoc for package "java.util.concurrent" is just plain wrong: "And because the happens-before relation is transitive, all actions of a thread prior to unlocking happen-before all actions subsequent to any thread locking that monitor". – Marcio Lucca Jul 11 '20 at 02:56
... Continuing: so true, maybe the test outside of the synchronized block might not see the change from a thread that currently has the monitor and is in the process of setting the "computed" flag. But as soon as the second thread enters the synchronized block, it should be able to see the change. – Marcio Lucca Jul 11 '20 at 03:03
Sorry, I'm reading the link you provided above in your original answer, @Eugene. This is messed up! Curse you, Java! lol. – Marcio Lucca Jul 11 '20 at 03:15
@MarcioLucca1) the _write_ of the volatile field `computed` is needed so that everything that is done _before_ it (i.e.: `value = supplier.get();`) is seen by some other thread that observes that write. Rather old, [but still quite good](http://jeremymanson.blogspot.com/2008/11/what-volatile-means-in-java.html). When some thread will see `computed == true`, it means `value != null`, 100%. The volatile read is an optimization so that you do not have to enter the synchronized block, what is also important is that on x86 this is a "free" optimization. – Eugene Jul 11 '20 at 03:17
@MarcioLucca 2) there is absolutely nothing wrong with the documentation that you quoted. Notice that it sort of makes it clear that you need to protected the sensitive code with the same lock, for both reads and writes, otherwise all bets are off. [here is more for your reading pleasure](https://stackoverflow.com/questions/61235213/thread-has-its-own-copy-of-data/61239326#61239326) – Eugene Jul 11 '20 at 03:19
@MarcioLucca you have triggered me to answer one more question [here](https://stackoverflow.com/questions/47638396/publishing-and-reading-of-non-volatile-field), after almost 3 years after I already gave one. I am very much thankful for that. – Eugene Jul 11 '20 at 03:54
VarHandle.setVolatile OR VarHandle.setRelease may solve the problem. but I am not sure. can you help me? – user10339780 Jul 12 '20 at 17:19
@user10339780 this is a fabulous question, at least to me. I think that you are asking : "can we replace the volatile write/read in the DCL pattern with release/acquire semantics?" To me, the answer is NO, but it's a lengthy answer. I really think you should ask this specific question separately. And accept this answer, if it helped. – Eugene Jul 12 '20 at 19:46

score 0 · Answer 2 · answered Jul 09 '20 at 03:07

The problem is, in theory, you have no control over what supplier.get() will return. It could be null, it could be a different value every time, etc. For this reason, I claim this code is not thread safe. Note also that "supplier" will only ever be null if they do:

new Lazy(null)

otherwise it will never be null. You might as well throw an exception in the constructor in this case

    public Lazy(Supplier<T> supplier) {
        if (supplier == null) {
            throw new IllegalArgumentException("'supplier' must not be null");
        }
        this.supplier = supplier;
    }

I'm not sure what you are trying to achieve here. If you want to intialize "value" lazily, only once, you could do something like:

if (value == null) {
    synchronize (this) {
        // test again to ensure no other thread initialized as we acquired monitor
        if (value == null) {
            value = supplier.get();
        }
    }
}

return value;

Thank you for reminding me. I wrote the wrong sample code. I have corrected the content — user10339780, Jul 09 '20 at 03:13
Ok, Now with your new code, supplier will never be null inside method get(), because method get() will never be executed before the constructor finishes. This is because in order for a thread to be able to use "lazySupplier.get()", first it must assign the fully constructed "Lazy" instance into reference "lazySupplier" and only then it would be able to invoke "lazySupplier.get()". This means, the constructor must always finish before any of the methods in the instance can be invoked. — Marcio Lucca, Jul 09 '20 at 03:19

Can this class be thread safe lazy loading, or is it as unsafe as "double checked locking"?

2 Answers2