2

I have read about many possible ways to create a singleton for the multithreaded environment in Java, like Enums, Double-check locking, etc.

I found a simple way that is also working fine and I unable to find its drawbacks or failure cases. May anyone explain when it may fail or why we should not choose this approach.

public final class MySingleton {
    public final static MySingleton INSTANCE = new MySingleton();
    private MySingleton(){} 
}

I am testing it with the below code and working fine:

public class MyThread {
    public static void main(String[] args) {

        for (int i = 0; i < 10000; i++) {
            Thread thread = new Thread(() -> {
                MySingleton singleton = MySingleton.INSTANCE;
                System.out.println(singleton.hashCode() + " " + Thread.currentThread().getName());
            });
            thread.start();
        }
    }
}

Every comment is appreciated.

dreamcrash
  • 47,137
  • 25
  • 94
  • 117
Bagesh Sharma
  • 783
  • 2
  • 9
  • 23
  • 2
    This is perfectly fine indeed :) The other patterns usually try to help to "load only if necessary", but if you're fine with eager loading, then this will suit your needs. That's usually what I go for too when I need a singleton. – sp00m Jan 25 '21 at 09:55
  • "I am testing it with the below code and working fine" What are you trying to show with that test? – Andy Turner Jan 25 '21 at 09:57
  • @AndyTurner I am printing the hashcode value from multiple threads and it's printing the same value from all threads. So I counting it as the same object. – Bagesh Sharma Jan 25 '21 at 10:00
  • Also note that this can be effectively still lazy-loaded if you just don't put anything else in that class. Classes themselves are only loaded on demand as well. – Thilo Jan 25 '21 at 10:10
  • @BageshSharma The answers you've gotten are mostly correct, but you should be aware of a troublesome error I ran into a few months ago. See my answer for details. – William Rosenbloom Jan 25 '21 at 10:28
  • 1
    @WilliamRosenbloom I have gone through your answer but did you get any root cause for that failure. – Bagesh Sharma Jan 25 '21 at 15:56
  • You could look into this https://shipilev.net/blog/2014/safe-public-construction/ – AnatolyG Jan 27 '21 at 14:05

4 Answers4

4

Your singleton is instantiated when the class is loaded, therefore when the main method is started, it is already instantiated and from a multithreading perspective is safe.

But there are some arguments why this might not be the best approach:

  • When instantiating your singleton throws an exception, you get a nasty classloading exception which is hard to analyze, especially when you don't have a debugger to attach.
  • When instantiating takes some time, so you might don't want to do this when the class is loaded in order to minimize the startup time of your application

And additionally using singletons is not a good design as you hardcode the dependency between the client (that uses this class) and the implementation of this class. So it is hard to replace your implementation with a mock for testing.

dreamcrash
  • 47,137
  • 25
  • 94
  • 117
Jens Baitinger
  • 449
  • 2
  • 8
3

Yes, this is a fine implementation of a singleton.

The test shows... something; but it doesn't really explain whether it's working or not. What you are trying to show (that only one instance is created) is essentially impossible to show with a test, because it's something that's guaranteed by the language spec.

See JLS 12, in particular JLS 12.4, which describes how classes are initialized.

For each class or interface C, there is a unique initialization lock LC. The mapping from C to LC is left to the discretion of the Java Virtual Machine implementation. The procedure for initializing C is then as follows:

  1. Synchronize on the initialization lock, LC, for C. This involves waiting until the current thread can acquire LC.
  2. If the Class object for C indicates that initialization is in progress for C by some other thread, then release LC and block the current thread until informed that the in-progress initialization has completed, at which time repeat this step.
  3. If the Class object for C indicates that initialization is in progress for C by the current thread, then this must be a recursive request for initialization. Release LC and complete normally.
  4. If the Class object for C indicates that C has already been initialized, then no further action is required. Release LC and complete normally.

...

So, classes are guaranteed to only be initialized once (if at all), because the initialization is done whilst holding a class-specific lock; the happens-before guarantees of acquiring and releasing that lock mean that the values of final fields are visible; so the INSTANCE field is guaranteed to be initialized once, and thus there is only one instance of MySingleton possible.


Note that your implementation is effectively the same as an enum:

public enum MySingleton {
    INSTANCE
}

Enums are really just syntactic sugar for classes. If you decompiled an enum class, it would look something like:

public class MySingleton {
    public static final MySingleton INSTANCE = new MySingleton(0);

    private MySingleton(int ordinal) { ... }
}
Andy Turner
  • 137,514
  • 11
  • 162
  • 243
1

It is perfectly fine and simple. Although you may check the trade offs listed below:

  1. May lead to resource wastage. Because instance of class is created always, whether it is required or not.

  2. CPU time is also wasted for creating the instance if it is not required.

  3. Exception handling is not possible.

Also you have to be sure that MySingleton class is thread-safe

See Java Singleton Design Pattern Practices with Examples

eHayik
  • 2,981
  • 1
  • 21
  • 33
0

Ah, I ran into some problems using exactly this approach a few months ago. Most people here will tell you this is fine, and they are mostly right, as long as you know the constructor won't throw an exception. What I'm about to describe is a common but unpredictable artifact of JVM caching conventions.

I had a singleton that was set up exactly the one you show in your question. Every time I attempted to run the program I got a NoClassDefFoundError. I had no idea what was going wrong until I found this post on Reddit, aptly titled Using static initialization for singletons is bad!

Just FYI, if you initialize a singleton with static initialization, e.g., private static final MyObject instance = new MyObject(); and there's business logic in your constructor, you can easily run into weird meta-errors, like NoClassDefFoundError. Even worse, it's unpredictable. Certain conditions, such as the OS, can change whether an error occurs or not. Static initialization also includes using a static block, "static{...}" , and using the enum singleton pattern.

The post also links to some Android docs about the cause of the problem.

The solution, which worked very well for me, is called double checked locking.

public class Example {

    private static volatile Example SINGLETON = null;
    private static final Object LOCK = new Object();

    private Example() {
        // ...do stuff...
    }

    public static Example getInstance() {
        if (SINGLETON == null) {
            synchronized (LOCK) {
                if (SINGLETON == null) {
                    SINGLETON = new Example();
                }
            }
        }
        return SINGLETON;
    }
}

For the most part, you can probably do it the simple way without problems. But, if your constructor contains enough business logic that you start getting NoClassDefFoundError, this slightly more complicated approach should fix it.

William Rosenbloom
  • 2,506
  • 1
  • 14
  • 37
  • 1
    Doesn't `SINGLETON` need to be `volatile` in the double-checked locking pattern though? – sp00m Jan 25 '21 at 10:39
  • 1
    @sp00m yes, good catch. I changed the code block accordingly. – William Rosenbloom Jan 25 '21 at 10:52
  • 2
    Double-checked locking is hard to get right, as evidenced by the fact it was initially wrong in the answer (even Josh Bloch got it wrong in the first imprint of *Effective Java* 3rd Ed). It should be eschewed for easier mechanisms; if you are using it because the constructor can fail, you should look to remove that possibility first. – Andy Turner Jan 25 '21 at 11:07
  • you can read of another way to get away from a `volatile read` [here](https://stackoverflow.com/questions/65171886/double-check-locking-without-volatile-but-with-varhandle-release-acquire) or even without `volatile` at all – Eugene Jan 25 '21 at 15:57