views:

338

answers:

6
public static MySingleton getInstance() {
 if (_instance==null) {
   synchronized (MySingleton.class) {
      _instance = new MySingleton();
   }
 }
 return _instance;
} 

1.is there a flaw with the above implementation of the getInstance method? 2.What is the difference between the two implementations.?

public static synchronized MySingleton getInstance() { 
 if (_instance==null) {
  _instance = new MySingleton();
 }

 return _instance;
} 

I have seen a lot of answers on the singleton pattern in stackoverflow but the question I have posted is to know mainly difference of 'synchronize' at method and block level in this particular case.

+4  A: 

1.is there a flaw with the above implementation of the getInstance method?

Yes, the synchronized keyword should wrap the if statement as well. If it's not then two or more threads could potentially get through to the creation code.

2.What is the difference between the two implementations.?

The second implementation is correct and from my point of view easier to understand.

Using synchronized at the method level for a static method synchronizes on the class, i.e. what you've done in sample 1. Using synchronized at the method level for an instance method synchronizes on the object instance.

Adrian
@OP: In this area, you'll find something called the "double-checked locking pattern". That's where you check, find the null, then synchronize, then check again (in case things have changed in the meantime), then create. It works in many languages, but *does not work in Java* unless you use a `volatile` field for the instance, which is 'way over the top. You're best off with your second implementation, especially with recent JVMs that handle entering syncrhonized blocks very efficiently indeed. More reading here: http://www.ibm.com/developerworks/library/j-jtp02244.html
T.J. Crowder
@OP re my comment above, I should have been more clear: It works in many *environments*, but not the JVM (unless you use a `volatile` field or its equivalent -- if any -- in the language you're using). Clarifying because these days, Java is just one of many languages that compiles down to Java bytecode and runs on the JVM (and similarly -- though more rarely -- there are some Java compilers that compile to machine code and don't use a JVM).
T.J. Crowder
+7  A: 

1.is there a flaw with the above implementation of the getInstance method?

It does not work. You can end up with several instances of your Singleton.

2.What is the difference between the two implementations.?

The second one works, but requires synchronization, which could slow down the system when you have a lot of accesses to the method from different threads.

The most straightforward correct implementation:

public class MySingleton{
    private static MySingleton _instance = new MySingleton();
    private MySingleton(){}
    public static MySingleton getInstance() { 
        return _instance;
    }
}

Shorter and better (safely serializable):

public enum MySingleton{
    INSTANCE;

    // methods go here
}

Lazy initialization of singletons is a topic that gets attention way out of proportion with its actual practical usefulness (IMO arguing about the intricacies of double-checked locking, to which your example is the first step, is nothing but a pissing contest).

In 99% of all cases, you don't need lazy initialization at all, or the "init when class is first referred" of Java is good enough. In the remaining 1% of cases, this is the best solution:

public enum MySingleton{
    private MySingleton(){}
    private static class Holder {
         static final MySingleton instance = new MySingleton();
    }
    static MySingleton getInstance() { return Holder.instance; }
}
Michael Borgwardt
enums aren't lazily-initialized, though
skaffman
Good points Michael. I like your straightforward implementation.
Adrian
@Tadeusz: thanks, corrected
Michael Borgwardt
Looks like you forgot both the type and the `static` keyword on the `_instance` member. Also, what's the purpose of synchronizing the `getInstance` method, if you're not doing a lazy-init? (Agreed that one rarely needs lazy init; but in those cases where one does...)
T.J. Crowder
Looks like you were editing as I was commenting. Still missing the type, though.
T.J. Crowder
@skaffman, @T.J.: got the enum approach confused with the holder class, corrected now
Michael Borgwardt
@Petar: Gave up and edited it myself, which I don't normally like to do when the original author is actively involved, but... That part is now correct.
T.J. Crowder
@Michael: That "best solution" involving the enum looks like serious enum abuse.
T.J. Crowder
@Michael: The more I look at it, the more I don't see any reason for that to be an enum rather than just a class. Either way, you're exploiting the classloader to do your synchronization for you (like this respondent some months back http://stackoverflow.com/questions/878577/are-java-static-initializers-thread-safe/879237#879237). It may be a valid approach (it's certainly interesting), though I'd question whether it actually performs better in real-world situations than working through a `volatile` member or just synchronizing the getter and living with it. But no need to throw `enum` at it!
T.J. Crowder
@Michael- I believe we need to have a private constructor to prevent direct instantion of MySingleTon.
Mohd Farid
"You can end up with several instances of your Singleton." the problem with DCL is more than just this, it's also possible to return partially-constructed instances of the object from the method (which is probably more severe in some cases than creating two instances).
matt b
@ T.J. Crowder: the massive advantage of the enum solution is that it ensures at a very low level that you cannot create additional instances. Specifically, it gives you safe serialization and is resilent against reflection. As an appeal to Autority: it's the solution favored by Joshua Block in the latest edition of Effective Java.
Michael Borgwardt
+2  A: 

The second one is thread safe, but it has the overhead of synchronized on every call, no matter if the instance is constructed or not. The first option has one major flaw that it doesn't have a check for if (_instance == null) in the synchronized block to prevent creating two instances.

Petar Minchev
+3  A: 

The first is flawed in two ways. As others mentioned here, multiple threads could get through

if (_instance==null) {

They would wait for each other, until the object is completely constructed, but they would do the instantiation and replace the reference in the variable.

The second flaw is a little more complicated. One thread could get into the constructor new MySingleton() and then the JVM switches to another thread. Another thread may check the variable for null, but that may contain a reference to a partially constructed object. So the other thread works on the partially constructed Singleton, that's also not good. So the first variant should be avoided.

The second variant should work fine. Don't care too much about efficiency, until you identify this clearly as blocker. Modern JVMs can optimize away unneeded synchronizations, so in real production-code this construct may never hurt performance.

Dishayloo
A: 

The various approaches to lazy-load singletons are discussed by Bob Lee in Lazy Loading Singletons and the "right" approach is the Initialization on Demand Holder (IODH) idiom which requires very little code and has zero synchronization overhead.

static class SingletonHolder {
  static Singleton instance = new Singleton();    
}

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

Bob Lee also explain when he wants to lazy load a singleton (during tests and development). Honestly, I'm not convinced there is a huge benefit.

Pascal Thivent
A: 

I would suggest the following implementation

public class MySingleTon
{

  private static MySingleton obj;

  //private Constructor
  private MySingleTon()
  {
  }


  public static MySingleTon getInstance()
  {
     if(obj==null)
     {
        synchronized(MySingleTon.class)
        {
         if(obj == null)
         {
             obj = new MySingleTon();
         }
        }
     }
     return obj;    
  }
}
Mohd Farid
This is still double-checked locking and therefore is still incorrect.
matt b