views:

313

answers:

6

Hi there:

I come up with this question when implementing singleton pattern in Java. Even though the example listed below is not my real code, yet very similar to the original one.

public class ConnectionFactory{
    private static ConnectionFactory instance;

    public static synchronized ConnectionFactory getInstance(){
        if( instance == null ){
            instance = new ConnectionFactory();
        }

        return instance;
    }

    private ConnectionFactory(){
        // private constructor implementation
    }
}

Because I'm not quite sure about the behavior of a static synchronized method, I get some suggestion from google -- do not have (or as less as possible) multiple static synchronized methods in the same class. I guess when implementing static synchronized method, a lock belongs to Class object is used so that multiple static synchronized methods may degrade performance of the system.

Am I right? or JVM use other mechanism to implement static synchronized method? What's the best practice if I have to implement multiple static synchronized methods in a class?

Thank you all!

Kind regards!

+6  A: 

The best approach (which makes as few changes in your code as possible) is to do like this:

public class ConnectionFactory{
    private static ConnectionFactory instance = new ConnectionFactory();

    public static ConnectionFactory getInstance(){
        return instance;
    }

    private ConnectionFactory(){
    }
}

As you can see, there is no real need in getInstance method now, so you can simplify the code to:

public class ConnectionFactory{
    public static final ConnectionFactory INSTANCE = new ConnectionFactory();

    private ConnectionFactory(){
    }
}

UPD about synchronization: the best way is synchronizing on a lock which is not visible to outer classes, i.e.:

public class ConnectionFactory{
    private static final Object lock = new Object();

    public static void doSmth() {
        synchronized (lock) {

           ...
        }
    }

    public static void doSmthElse() {
        synchronized (lock) {

           ...
        }
    }
}

There are many discussions about "why synchronizing on this is a bad idea" (like this one), I think that the same is actual for synchronizing on class.

Roman
how would it create the `instance`, if the only constructor throws an exception?
unbeli
@unbeli: it was my bad, corrected. But if you don't even need an instance if was a correct solution.
Roman
@unbeli: by using a private static factory method, or a static initializer block. Unfortunately, all those discussions about the Java memory model and double-checked locking have polluted the internet with hundreds of code samples that now give novices the impression that lazily initializing singletons in the getInstance() method actually is a good idea or even the norm.
Michael Borgwardt
your corrected way is ok, except that it's not lazy, so it does not exactly address the OP problem
unbeli
@Michael, perhaps you haven't seen the code before it was corrected, so you can't really discuss that. Thank you.
unbeli
@unbeli: it's lazy enough. It creates instance only when any ConnectionFactory's field or method is used and do it only once.
Roman
@Roman - I'm not sure about your last code sample. What benefit is there to using a lock object over the intrinsic lock of the Class object?
Scobal
@Scobal: read linked discussion. In short: it's possible to make a deadlock with synchronizing on ConnectionFactory.class if someone else in another class will put `synchronized (ConnectionFactory.class){}`.
Roman
@Roman - Missed that link, apologies!
Scobal
@Roman agreed, good enough for most of the cases.
unbeli
@Roman: I would make the `instance` (or `INSTANCE`) variable final... at least should be so if it's public. (I would prefer to stay with getInstance(), but that is another story)
Carlos Heuberger
@Carlos Heuberger: agreed. Recently, I almost have a rule of thumb to make final everything what can be final.
Roman
+2  A: 

Yes, static methods are synchronized on their class object. I wouldn't worry about performance here, as probably this will not be your performance hot spot. Do it simple, optimize when and where you need to.

unbeli
+2  A: 

Static synchronized methods use the lock on the class. In the case of your example it would be accessing the lock on the ConnectionFactory class object. Best practice is not to hold onto locks for longer than you have to. Whether you have multiple synchronized methods is not a problem in itself.

Nathan Hughes
A: 
public class ConnectionFactory{
    private static ConnectionFactory instance;
    private static Object instanceLock = new Object();

    public static ConnectionFactory getInstance(){
        synchronized(instanceLock) {
            if( instance == null ){
                instance = new ConnectionFactory();
            }
        }
        return instance;
    }

    private ConnectionFactory(){
        // private constructor implementation
    }
}
glowcoder
NullPointerException guaranteed
unbeli
What if I have multiple static synchronized methods? Should I use an Object for each method? Thanx!
Summer_More_More_Tea
It depends on what kind of methods they are. They may or may not have (or require) internal synchronization. That's really an open ended question. @unbeli I've used this pattern before in the past without trouble.
glowcoder
@glowcoder, you've edited your answer and added the `new Object()` call. Now it's ok, no NPE. When you make a mistake, someone points it out and you correct it, please don't pretend it was like that from the beginning.
unbeli
I would not recommend this pattern. I wont talk about thread safe lazy intiailization because, well, its talked about way too much. But there are much better ways of doing what you need. See Scobal's solutions. The enum singleton pattern, and static class holding idiom are much better
John V.
And what happens if there are static methods other than getInstance that are called at a time where the instance couldn't be created? It will bomb when it shouldn't. Lazy initialization is straightforward, simple, and effective.
glowcoder
+3  A: 

There are a couple of ways you can create a singleton.

One recommended way is to use an enum (guaranteed to only create one instance):

public enum ConnectionFactory {

  INSTANCE;

}

Or you can create it statically when the class loads:

public class ConnectionFactory {

  private static ConnectionFactory INSTANCE = new ConnectionFactory();

  private ConnectionFactory() {}

  public static ConnectionFactory getInstance() {
    return INSTANCE;
  }    

}

If you need to lazily load it you can use this idiom (rather than the double checked locking anti-pattern )

public class ConnectionFactory {

  private static class ConnectionFactoryHolder {
    private static ConnectionFactory INSTANCE = new ConnectionFactory();
  }

  public static ConnectionFactory getInstance() {
    return ConnectionFactoryHolder.INSTANCE;
  }

}
Scobal
A: 

Effective Java recommends using Enums to create singleton. So you code would look something like this:

public enum ConnectionFactory{
INSTANCE;

// Other factory methods go here.

}

}

Shikhar