CODEWITHSUNDEEP
javathread-safety
user10339780
user10339780

Reputation: 963

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

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?

Upvotes: 2

Views: 199

Answers (2)

Eugene
Eugene

Reputation: 120858

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;
     }

}

Upvotes: 3

Marcio Lucca
Marcio Lucca

Reputation: 380

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;

Upvotes: 0

Related Questions