26

What is the preferred way to work with Singleton class in multithreaded environment?

Suppose if I have 3 threads, and all of them try to access getInstance() method of singleton class at the same time -

  1. What would happen if no synchronization is maintained?
  2. Is it good practice to use synchronized getInstance() method or use synchronized block inside getInstance().

Please advise if there is any other way out.

didierc
  • 14,572
  • 3
  • 32
  • 52
vivekj011
  • 1,129
  • 3
  • 16
  • 23
  • Just so you know, the answer you accepted is a broken [anti-pattern](http://en.wikipedia.org/wiki/Anti-pattern) - it *seems* threadsafe, but actually isn't. It's all explained [here](http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html). – Bohemian Jun 17 '12 at 15:30
  • 1
    possible duplicate of [Efficient way to implement singleton pattern in Java](http://stackoverflow.com/questions/70689/efficient-way-to-implement-singleton-pattern-in-java) – bestsss Jun 17 '12 at 15:36

9 Answers9

43

If you're talking about threadsafe, lazy initialization of the singleton, here is a cool code pattern to use that accomplishes 100% threadsafe lazy initialization without any synchronization code:

public class MySingleton {

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

     private MySingleton () {}

     public static MySingleton getInstance() {
         return MyWrapper.INSTANCE;
     }
}

This will instantiate the singleton only when getInstance() is called, and it's 100% threadsafe! It's a classic.

It works because the class loader has its own synchronization for handling static initialization of classes: You are guaranteed that all static initialization has completed before the class is used, and in this code the class is only used within the getInstance() method, so that's when the class loaded loads the inner class.

As an aside, I look forward to the day when a @Singleton annotation exists that handles such issues.

Edited:

A particular disbeliever has claimed that the wrapper class "does nothing". Here is proof that it does matter, albeit under special circumstances.

The basic difference is that with the wrapper class version, the singleton instance is created when the wrapper class is loaded, which when the first call the getInstance() is made, but with the non-wrapped version - ie a simple static initialization - the instance is created when the main class is loaded.

If you have only simple invocation of the getInstance() method, then there is almost no difference - the difference would be that all other sttic initialization would have completed before the instance is created when using the wrapped version, but this is easily dealt with by simply having the static instance variable listed last in the source.

However, if you are loading the class by name, the story is quite different. Invoking Class.forName(className) on a class cuasing static initialization to occur, so if the singleton class to be used is a property of your server, with the simple version the static instance will be created when Class.forName() is called, not when getInstance() is called. I admit this is a little contrived, as you need to use reflection to get the instance, but nevertheless here's some complete working code that demonstrates my contention (each of the following classes is a top-level class):

public abstract class BaseSingleton {
    private long createdAt = System.currentTimeMillis();

    public String toString() {
        return getClass().getSimpleName() + " was created " + (System.currentTimeMillis() - createdAt) + " ms ago";
    }
}

public class EagerSingleton extends BaseSingleton {

    private static final EagerSingleton INSTANCE = new EagerSingleton();

    public static EagerSingleton getInstance() {
        return INSTANCE;
    }
}

public class LazySingleton extends BaseSingleton {
    private static class Loader {
        static final LazySingleton INSTANCE = new LazySingleton();
    }

    public static LazySingleton getInstance() {
        return Loader.INSTANCE;
    }
}

And the main:

public static void main(String[] args) throws Exception {
    // Load the class - assume the name comes from a system property etc
    Class<? extends BaseSingleton> lazyClazz = (Class<? extends BaseSingleton>) Class.forName("com.mypackage.LazySingleton");
    Class<? extends BaseSingleton> eagerClazz = (Class<? extends BaseSingleton>) Class.forName("com.mypackage.EagerSingleton");

    Thread.sleep(1000); // Introduce some delay between loading class and calling getInstance()

    // Invoke the getInstace method on the class
    BaseSingleton lazySingleton = (BaseSingleton) lazyClazz.getMethod("getInstance").invoke(lazyClazz);
    BaseSingleton eagerSingleton = (BaseSingleton) eagerClazz.getMethod("getInstance").invoke(eagerClazz);

    System.out.println(lazySingleton);
    System.out.println(eagerSingleton);
}

Output:

LazySingleton was created 0 ms ago
EagerSingleton was created 1001 ms ago

As you can see, the non-wrapped, simple implementation is created when Class.forName() is called, which may be before the static initialization is ready to be executed.

Community
  • 1
  • 1
Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • It's also wretched, wretched, wretched if you have to write unit tests for a class that uses one of these. –  Jun 17 '12 at 15:14
  • @BenBrunk Why it is a problem? It should work just fine. What problems have you had? – Bohemian Jun 17 '12 at 15:14
  • Another valid way cited in the other response that I consider the best is the use of Enum. Bohemian solution is concise and elegant, but the enum way offer as a + that it give you for free serialization compliance – Andrea Parodi Jun 17 '12 at 15:17
  • Bohemian, if you are testing a class that uses one of these, there is no way to mock the singleton. I manage a lot of legacy code and there are techniques for dealing with singletons, but it is better to avoid the whole thing in new code or use an injection framework so that you can write good unit tests without compromises. –  Jun 17 '12 at 15:22
  • @Bohemian Why do you need the wrapper class? Couldn't you just define INSTANCE as a static final member in the MySingleton class? – Justin Jun 17 '12 at 15:32
  • no point (whatsoever) in the wrapper class with this example. – bestsss Jun 17 '12 at 15:38
  • @AndreaParodi, enums blow for this regardless what Mr Bloch says, they are serializable and you DO NOT want it w/ singleton. – bestsss Jun 17 '12 at 15:39
  • @bestsss The wrapper class is needed to delay instantiation until `getInstance()` is called. That is what "lazy initialization" means. – Bohemian Jun 17 '12 at 15:40
  • :D well, the only way to initialize MySingleton is via its the only public method getInstance() in this case. Note I actually know how the class loading/initialization works. – bestsss Jun 17 '12 at 15:43
  • @Justin The wrapper class is needed to delay instantiation until getInstance() is called. That is what "lazy initialization" means. If you defined a static `INSTANCE` field on `MySingleton`, it would be initialized at the first reference to the `MySingleton` class (rather than the execution of the `getInstance()` method), so reference to any class just *using* the `MySingleton` class would cause the `INSTANCE` field to be initialized, which may be too soon for the code to handle – Bohemian Jun 17 '12 at 15:44
  • @bestsss fyi, more info in my other recent comment – Bohemian Jun 17 '12 at 15:44
  • How could you use MySingleton beside using `Class.forName()`, if the class has no other public static methods or public static fields, the wrapper is pointless. – bestsss Jun 17 '12 at 15:48
  • @Bohemian So, if you only want a singleton you can use a static final member but if you want a singleton with lazy init then use a wrapper class. – Justin Jun 17 '12 at 16:35
  • @bestsss (OMG... I thought it was obvious, but apparently not): I only coded the parts of the `MySingleton` class that were relevant to the question/answer - I omitted the rest of the class for clarity. You can add whatever you like to it and make it as complex as you like. And just so it's clear, the wrapper class is *essential* for correct, thread-safe, lazy initialization behaviour. Please read the last paragraph of my answer carefully and try to understand it. It is a subtle thing going on here, but trust me, this code absolutely works, and works well. Try debugging this code yourself. – Bohemian Jun 18 '12 at 01:03
  • Most singletons rarely have any other public static methods (not used in conjunction w/ getInstance()) or non inalineable constants. That's the whole point. Again, I am very well aware how it's implemented on JVM level - class loading is a trap that once the code is loaded it gets replaced w/ code direct accessing the now-constant, similar results can be achieved via invokeDynamic. Last: almost never an inner class helps - just 'static final instance=new XXX()` is enough AND lazy. – bestsss Jun 18 '12 at 06:23
  • @bestsss Firstly, by "other methods" I meant of course *instance* methods (not static methods - I can't believe I had to say that). Secondly, `static final instance = new XXX()` is **not** lazy!!! It beyond me why you're not getting this. You don't seem even the most basic understanding of this stuff. I want to help, and I don't want to be rude, but I can't explain it any more clearly. If you *still* think you're right and I'm wrong, you're going to have to read up elsewhere - don't comment here saying that I'm wrong, because I'm absolutely 100% correct. – Bohemian Jun 18 '12 at 07:09
  • *because I'm absolutely 100% correct.* - no, you are not. `static final instance = new XXX()` is lazy, if you wish put a system log when the c-tor is called. Try w/ and w/o the inner class. Instance methods DO NOT help w/ the lazy init. The lazy init. can happen ONLY if there are other `public static` methods (or fields) that are used w/o regarding the `instance`, i.e. some Util methods. If there is no public c-tor AND no public static methods besides getInstance() -- the inner class is useless. You can read the JLS or try experimenting yourself, but you are not 100% correct.` – bestsss Jun 18 '12 at 07:39
  • Why it is lazy: classes are initialized () on their 1st use: if there is no public c-tor and no public methods - the 1st use is getInstance() -- hence lazy. – bestsss Jun 18 '12 at 07:41
  • @bestsss Interesting. I posted some discussion and working code the demonstrates that I am correct (ie there is a difference using the wrapped class), however the circumstances when the difference matters wasn't quite what I expected. So I have learned something about class loading. You can copy-paste my code and run it yourself to see. As per my comments in the post, I will admit that when the code is used "normally" there is still a difference, but it is trivial. – Bohemian Jun 18 '12 at 18:49
  • `Class.forName(String)` resolves the class, it is meant to. It should be used if the needs to be called, e.g. JDBC drivers. for all other cases `Class.forName(className, false, classLoader)` should be used. Bohemian, try like that the result will differ. Also, I did mention "beside using Class.forName()". – bestsss Jun 18 '12 at 19:18
18

The task is non-trivial in theory, given that you want to make it truly thread safe.

A very nice paper on the matter is found @ IBM

Just getting the singleton does not need any sync, since it's just a read. So, just synchronize the setting of the Sync would do. Unless two treads try to create the singleton at start up at the same time, then you need to make sure check if the instance is set twice (one outside and one inside the sync) to avoid resetting the instance in a worst case scenario.

Then you might need to take into account how JIT (Just-in-time) compilers handle out-of-order writes. This code will be somewhat near the solution, although won't be 100% thread safe anyway:

public static Singleton getInstance() {
    if (instance == null) {
        synchronized(Singleton.class) {      
            Singleton inst = instance;         
            if (inst == null) {
                synchronized(Singleton.class) {  
                    instance = new Singleton();               
                }
            }
        }
    }
    return instance;
}

So, you should perhaps resort to something less lazy:

class Singleton {
    private static Singleton instance = new Singleton();

    private Singleton() { }

    public static Singleton getInstance() {
        return instance;
    }
}

Or, a bit more bloated, but a more flexible way is to avoid using static singletons and use an injection framework such as Spring to manage instantiation of "singleton-ish" objects (and you can configure lazy initialization).

Milan
  • 147
  • 1
  • 4
  • 16
Petter Nordlander
  • 22,053
  • 5
  • 50
  • 84
  • 5
    Just so you know, this is a broken pattern. See [this article](http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html). – Bohemian Jun 17 '12 at 15:29
  • 2
    you need volatile for the `instance` field and the double locking inside is useless as the JVM is free to coarsen locks. Volatile reads are practically free on most of the hardware, so don't be afraid of volatile *reads*. Writes are significantly more expensive as they require to empty to CPU buffers. This way presented the answer is plain wrong. – bestsss Jun 17 '12 at 17:25
  • @Bohemian People should stop saying this is a broken pattern. It has been broken in the past in Java but no more with the correction made to `volatile` keyword in Java 5. OK, it's a tricky pattern that must be well understand, but it works now and is sometimes very useful. Here a complex but insightful article on the subject: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html. – Doc Davluz Apr 30 '20 at 07:05
  • Is the 2nd simple implementaion with `private static Singleton instance = new Singleton();` thread-safe? – Ekaterina May 18 '20 at 11:15
5

You need synchronization inside getInstance only if you initialize your singleton lazily. If you could create an instance before the threads are started, you can drop synchronization in the getter, because the reference becomes immutable. Of course if the singleton object itself is mutable, you would need to synchronize its methods which access information that can be changed concurrently.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
3

This question really depends on how and when your instance is created. If your getInstance method lazily initializes:

if(instance == null){
  instance = new Instance();
}

return instance

Then you must synchronize or you could end up with multiple instances. This problem is usually treated in talks on Double Checked Locking.

Otherwise if you create a static instance up front

private static Instance INSTANCE = new Instance();

then no synchronization of the getInstance() method is necessary.

nsfyn55
  • 14,875
  • 8
  • 50
  • 77
3

The best way as described in effective java is:

public class Singelton {

    private static final Singelton singleObject = new Singelton();

    public Singelton getInstance(){
        return singleObject;
    }
}

No need of synchronization.

sorabh solanki
  • 480
  • 1
  • 7
  • 18
1

Nobody uses Enums as suggested in Effective Java?

Florian Salihovic
  • 3,921
  • 2
  • 19
  • 26
  • 1
    I will give you an example why Effective Java and Mr. Bloch are wrong. Imagine your singleton is defined as interface and there are few possible impl. depending on configuration parameters, Now Imagine some class keeps a reference to the singleton in a member field and that class is serialized... upon deserialization you wake up w/ 2(!) different instances of the singleton. Thanks to the immense beauty of the *free* serialization. Singletons MUST NOT be serializable, ever. (and yes I had to dig into a heap dump b/c someone thought to create serializable singletons) – bestsss Jun 17 '12 at 16:04
  • Enums use readResolve() to prevent two instances of an enum value from being visible. – NamshubWriter Dec 31 '14 at 00:27
0

If you are sure that your java runtime is using the new JMM (Java memory model, probably newer than 5.0), double check lock is just fine, but add a volatile in front of instance. Otherwise, you'd better use static internal class as Bohemian said, or Enum in 'Effective Java' as Florian Salihovic said.

0

For simplicity, I think using enum class is a better way. We don't need to do any synchronization. Java by construct, always ensure that there is only one constant created, no matter how many threads are trying to access it.

FYI, In some case you need to swap out singleton with other implementation. Then we need to modify class, which is violation of Open Close principal.Problem with singleton is, you can't extend the class because of having private constructor. So, it's a better practice that client is talking via interface.

Implementation of Singleton with enum class and Interface:

Client.java

public class Client{
    public static void main(String args[]){
        SingletonIface instance = EnumSingleton.INSTANCE;
        instance.operationOnInstance("1");
    }
}

SingletonIface.java

public interface SingletonIface {
    public void operationOnInstance(String newState);
}

EnumSingleton.java

public enum EnumSingleton implements SingletonIface{
INSTANCE;

@Override
public void operationOnInstance(String newState) {
    System.out.println("I am Enum based Singleton");
    }
}
Jaimin Patel
  • 183
  • 1
  • 13
0

The Answer is already accepted here, But i would like to share the test to answer your 1st question.

What would happen if no synchronization is maintained?

Here is the SingletonTest class which will be completely disaster when you run in multi Threaded Environment.

/**
 * @author MILAN
 */
public class SingletonTest
{
    private static final int        PROCESSOR_COUNT = Runtime.getRuntime().availableProcessors();
    private static final Thread[]   THREADS         = new Thread[PROCESSOR_COUNT];
    private static int              instancesCount  = 0;
    private static SingletonTest    instance        = null;

    /**
     * private constructor to prevent Creation of Object from Outside of the
     * This class.
     */
    private SingletonTest()
    {
    }

    /**
     * return the instance only if it does not exist
     */
    public static SingletonTest getInstance()
    {
        if (instance == null)
        {
            instancesCount++;
            instance = new SingletonTest();
        }
        return instance;
    }

    /**
     * reset instancesCount and instance.
     */
    private static void reset()
    {
        instancesCount = 0;
        instance = null;
    }

    /**
     * validate system to run the test
     */
    private static void validate()
    {
        if (SingletonTest.PROCESSOR_COUNT < 2)
        {
            System.out.print("PROCESSOR_COUNT Must be >= 2 to Run the test.");
            System.exit(0);
        }
    }

    public static void main(String... args)
    {
        validate();
        System.out.printf("Summary :: PROCESSOR_COUNT %s, Running Test with %s of Threads. %n", PROCESSOR_COUNT, PROCESSOR_COUNT);
        
        long currentMili = System.currentTimeMillis();
        int testCount = 0;
        do
        {
            reset();

            for (int i = 0; i < PROCESSOR_COUNT; i++)
                THREADS[i] = new Thread(SingletonTest::getInstance);

            for (int i = 0; i < PROCESSOR_COUNT; i++)
                THREADS[i].start();

            for (int i = 0; i < PROCESSOR_COUNT; i++)
                try
                {
                    THREADS[i].join();
                }
                catch (InterruptedException e)
                {
                    e.printStackTrace();
                    Thread.currentThread().interrupt();
                }
            testCount++;
        }
        while (instancesCount <= 1 && testCount < Integer.MAX_VALUE);
        
        System.out.printf("Singleton Pattern is broken after %d try. %nNumber of instances count is %d. %nTest duration %dms", testCount, instancesCount, System.currentTimeMillis() - currentMili);
    }
}

Output of the program is clearly shows that you need handle this using getInstance as synchronized or add synchronized lock enclosing new SingletonTest.

Summary :: PROCESSOR_COUNT 32, Running Test with 32 of Threads. 
Singleton Pattern is broken after 133 try. 
Number of instance count is 30. 
Test duration 500ms
Milan
  • 147
  • 1
  • 4
  • 16