8

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!

Nathan Hughes
  • 94,330
  • 19
  • 181
  • 276
Summer_More_More_Tea
  • 12,740
  • 12
  • 51
  • 83
  • 2
    Is your real code also concerned with lazy initialization? Because that's usually a waste of code and developer time; initializing the field in its declaration is the correct thing to do 99% of the time and makes synchronization unnecessary. – Michael Borgwardt Jun 02 '10 at 13:34

5 Answers5

7

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.

Community
  • 1
  • 1
Roman
  • 64,384
  • 92
  • 238
  • 332
  • how would it create the `instance`, if the only constructor throws an exception? – unbeli Jun 02 '10 at 13:23
  • @unbeli: it was my bad, corrected. But if you don't even need an instance if was a correct solution. – Roman Jun 02 '10 at 13:27
  • @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 Jun 02 '10 at 13:31
  • your corrected way is ok, except that it's not lazy, so it does not exactly address the OP problem – unbeli Jun 02 '10 at 13:32
  • @Michael, perhaps you haven't seen the code before it was corrected, so you can't really discuss that. Thank you. – unbeli Jun 02 '10 at 13:33
  • @unbeli: it's lazy enough. It creates instance only when any ConnectionFactory's field or method is used and do it only once. – Roman Jun 02 '10 at 13:37
  • @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? – Mark Pope Jun 02 '10 at 13:42
  • 1
    @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 Jun 02 '10 at 13:46
  • @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) – user85421 Jun 02 '10 at 14:13
  • @Carlos Heuberger: agreed. Recently, I almost have a rule of thumb to make final everything what can be final. – Roman Jun 02 '10 at 14:20
3

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;
  }

}
Mark Pope
  • 11,244
  • 10
  • 49
  • 59
2

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
  • 29,501
  • 5
  • 55
  • 57
2

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
  • 94,330
  • 19
  • 181
  • 276
0

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 Mishra
  • 1,965
  • 2
  • 13
  • 14