Is it safe to use writeLock without readlock ?

Question

Assuming this is a "singleton" implementation: Am I guaranteed that this will only call productCatalogLoader.load() once, and also that no nullpointers can happen ? Any way to make this simpler ?

private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
private ProductCatalog productCatalog;

public ProductCatalog get() {
        if (this.productCatalog == null) {
            reload();
         }
        return this.productCatalog;
}                                                         

public void reload() {

    lock.writeLock().lock();
    try {
        if (this.productCatalog != null) return;
        this.productCatalog = productCatalogLoader.load();
    } finally {
        lock.writeLock().unlock();
    }
}

}

Edit: This was a modestly successful attempt reduce much more complex code to a simple question sample. Several people caught on to the fact that my singleton was too complex ;) (Actually there's also a quartz timer calling reload, but the "reload" implementation is a bit different IRL). I got the answer I needed anyway, broken double checked locking.

Answer 1

The best way to make it simpler, IMO, is to use a "normal" lock unless you really have good evidence that it's causing a bottleneck:

private final Object lock = new Object();
private ProductCatalog productCatalog;

public ProductCatalog get() {
    synchronized (lock) {
        if (this.productCatalog == null) {
            this.productCatalog = productCatalogLoader.load();
        }
        return this.productCatalog;
    }
}

In the vast majority of cases, this will be good enough, and you don't need to worry about memory model technicalities.

EDIT: As to whether reading the data changed in the write lock without acquiring the read lock is safe - I suspect not, but I wouldn't like to say for certain. Is there any benefit in your approach to using normal (and safe on Java 1.5+, if you're careful) double-checked locking using a volatile variable?

private final Object lock = new Object();
private volatile ProductCatalog productCatalog;

public ProductCatalog get() {
    if (this.productCatalog == null) {
        synchronized (lock) {
            if (this.productCatalog == null) {
                this.productCatalog = productCatalogLoader.load();
            }
        }
    }
    return this.productCatalog;
}

I believe that's the lazy initialization technique recommended in Effective Java 2nd edition, for situations where static initializers aren't enough. Note that productCatalog has to be volatile for this to work - and I think that's effectively what you're missing by not taking out the read lock in your code.

Answer 2

No, in general it is not safe... The writelock only guarantees that no-one else will be able to get a readlock or writelock until the writelock has been released. Not taking the readlock in your code is like having the reload method synchronized but not the get method. I sort of agree with Jon Skeet here but it depends a lot on the usage.

If you have a lot threads that calls get at more or less the same time, you probably want a readlock rather than having it synchronized.

Edit: Btw, there is rather good example on how to do it in Effective Java 2nd Edition... I also have to correct myself here, given the context, synchronization will work just as well if done in a smart way (and the writelock will probably work too in this context).

Answer 3

Isn't it simpler, if you are trying to make a singleton to do:

private final ProductCatalog productCatalog = productCatalogLoader.load();
public ProductCatalog get() {
   return this.productCatalog;
}

If there is some reason why you can't make ProductCatalog a final member, then either using synchronized per Jon Skeet's answer or your original code should work as long as the member productCatalog is not accessed except through that get() call, or your reload().

Answer 4

this looks like broken double checked locking. the problem is this.productCatalog can be assigned before the object is fully contructed.

say we have

var foo = new ProductCatalog();
foo.blah = "blah blah";
this.productCatalog = foo;

the can be reordered as

var foo = new ProductCatalog();
this.productCatalog = foo;
foo.blah = "blah blah";