Thread safe singleton, locks

Question

I have a Singleton class which is a thread safe, do I need to lock it methods?

        private static volatile JsonWriter instance;
private static final Object mutex = new Object();

ReentrantReadWriteLock lock = new ReentrantReadWriteLock();

private JsonWriter() {
}

public static JsonWriter getInstance() {
    JsonWriter result = instance;
    if (result == null) {
        synchronized (mutex) {
            result = instance;
            if (result == null) {
                instance = result = new JsonWriter();
            }
        }
    }
    return result;
}

do I need to lock each method like this to ensure thread safety?

    public void write(String filePath, ArrayNode content) throws IOException {
    lock.writeLock().lock();
    File file = new File(MASTER_DIR + "/" + filePath);
    mapper.writerWithDefaultPrettyPrinter().writeValue(Files.newOutputStream(file.toPath()), content);

}

Dan · Accepted Answer · 2022-06-01T14:54:57.297

3

The best performing and thread-safe Singleton implementation is the William Pugh Singleton. You don't need synchronized blocks and ReentrantReadWriteLock.

The William Pugh implementation ensures multi-thread safety and the best performances as it avoids eager creation. In fact, the static member INSTANCE, initialized at class level, will be created only when the nested class is loaded by the class loader, i.e. when the nested class will be actually used. In the following implementation, this can only happen when the getInstance() method is invoked. In fact, conversely from the EagerSingleton, this model actually allows using the enclosing class without causing an eager instantiation. This means that any other method offered by the enclosing class can be safely used without initializing INSTANCE; only the getInstance() method will cause it.

Your implementation could look like this:

class JsonWriter {

    private JsonWriter() {}

    /* ... other methods ... */

    public static JsonWriter getInstance() {
        return SingletonHelper.INSTANCE;
    }

    private static class SingletonHelper {
        private static final JsonWriter INSTANCE = new JsonWriter();
    }
}

A particular thanks to @Holger, who contributed with important clarifications and remarks in the comments.

edited Jun 01 '22 at 14:54

answered May 31 '22 at 14:45

Dan

3,647
5
20
26

2

You get the same effect when you place the `INSTANCE` field directly into the `JsonWriter` and remove the nested `SingletonHelper` class. And technically, it’s the opposite of what you say, a nested class is not “simply a static member”, a nested class is rather an ordinary package-private class with additional attributes telling about the nesting relationship. For the class initialization time, this relationship is irrelevant. – Holger Jun 01 '22 at 08:12
@Holger Here we're talking about instantiation moments and when a class is actually loaded by the Class Loader. A nested static private class is not an ordinary package-private class. This is not accessible from any class outside the outer class; even from classes inside the outer class' package. Besides the loading model for static class does correspond to what I wrote. Here are a few articles to back up my answer: https://www.journaldev.com/1377/java-singleton-design-pattern-best-practices-examples#bill-pugh-singleton https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom – Dan Jun 01 '22 at 08:37
2

This is Cargo Cult programming. Just copying the same code over and over without understanding. First of all, you are confusing *loading* time and *initialization* time. All that matters, is [when the class is initialized](https://docs.oracle.com/javase/specs/jls/se17/html/jls-12.html#jls-12.4.1) as that’s when the `INSTANCE = new JsonWriter()` initializer will be executed. The class `JsonWriter` will be initialized when `getInstance()` is executed the first time and that’s already sufficient to initialize an `INSTANCE` field lazily. The nested class does not add anything useful. – Holger Jun 01 '22 at 08:45
@Holger got a bit of time to rephrase my answer. Those were my personal notes btw, "no cargo culture" here. They probably did not explain the concept correctly and got confusing at some point. Especially around the passage "simply a static member". Thanks for your input btw! It got me the chance to revise the topic and improve my answer. Let me know if you have any other observation to make. – Dan Jun 01 '22 at 11:16
2

As said, don’t confuse *loading* and *initialization*. The loading time is intentionally unspecified and irrelevant. What matters, is the initialization time. I already linked to the specification in my previous comment. The field will be initialized when the class is used, i.e. when the field `INSTANCE` is accessed the first time. But the same would apply if there was no nested class involved. The class `JsonWriter` is also only initialized when it is actually used, i.e. the first time `getInstance()` is invoked. A nested class would be helpful when there were other, unrelated static members. – Holger Jun 01 '22 at 11:29
@Holger ok. It's way clearer now. I tested the code with some test cases like in the example in your link. Basically, even if I rewrote the JSonWriter with no nested class and declared a static block to print the content of INSTANCE, I noticed that nothing is printed if I leave the main empty. I was actually expecting the program to print null, as I thought that the class was going to be loaded immediately at the start. In fact, if I declare a dummy empty method within the new JSonWriter and then call it in the main method, then the class is loaded and the print shows up. – Dan Jun 01 '22 at 12:19
@Holger now I also better understand when you said that the nested static class implementation makes sense when you have multiple fields. In case of a single instance to retrieve, it doesn't matter whether the instance is initialized within a class or a static nested class. With one single instance to retrieve both cases coincide, so there is no need for a nested static class. This has been very insightful. Thank you! – Dan Jun 01 '22 at 12:24

score 2 · Answer 2 · answered May 31 '22 at 14:45

1 Please dont use synchronized and Lock in one class

2 If you want to use synchronized:

private static volatile JsonWriter instance;   

private JsonWriter() {
}

public synchronized static JsonWriter getInstance() {
    JsonWriter result = instance;
    if (result == null) {
        instance = new JsonWriter();
    }
    return result;
}

3 Or you can use Locks:

private static volatile JsonWriter instance;
private static final Object mutex = new Object();

ReentrantReadWriteLock lock = new ReentrantReadWriteLock();

private JsonWriter() {}

public static JsonWriter getInstance() {
    lock.writeLock().lock();
    JsonWriter result = instance;
    if (result == null) {
       instance = result = new JsonWriter();
      }
    lock.writeLock().unlock();
    return result;
}

Thread safe singleton, locks

2 Answers2