Another way to write getInstance for singleton?

[Dave Alvarado]

Hi,

Currently, in my singleton, I have this ...

  public static synchronized YouthfulDriverDatabase getInstance() {
    if(youthfulDriverDatabase == null) {
      youthfulDriverDatabase = new YouthfulDriverDatabase();
    }
    return youthfulDriverDatabase;
  }

Yet I'm told by my code checker, "Use block level rather than method level synchronization". Sounds simple enough. But before I throw a syncrhonized(this) around everything, I wanted to ask what the proper way to write this getInstance method would be without a synchronized at the method level.

Thanks, - Dave

[Robert Jester]

Try googling 'double check idiom java'

[Paul Clapham]

Taking that advice would result in this code:

public static YouthfulDriverDatabase getInstance() {
  synchronized {
    if(youthfulDriverDatabase == null) {
      youthfulDriverDatabase = new YouthfulDriverDatabase();
    }
    return youthfulDriverDatabase;
  }
}

That synchronizes the same code on the same monitor, so it works exactly the same. I don't know why it's "better", though. Maybe the code checker comes with some help text explaining its recommendations?

[John de Michele]

Dave:

Actually, you may want to avoid the double check idiom: link.

John.

[Robert Jester]

John de Michele wrote:Dave:

Actually, you may want to avoid the double check idiom: link.

John.

John, please read the whole article. It should provide some alternatives.

[John de Michele]

"Sand Lord":

I did read the article. That's why I used the word 'may'. With more information, Dave may want to revise his decision (or not). BTW, you'll probably want to change your user name, since the site rules require real name user names.

John.

[Rob Spoor]

You can read that full policy here.

[Embla Tingeling]

Dave Alvarado wrote:
Currently, in my singleton, I have this ...

There's a Singleton variation which avoids most of the implementation issues. It's based on using an enum, like

public enum MySingleton {
   INSTANCE;
   //
   public void doSomething() {
   }
}
//
MySingleton.INSTANCE.doSomething(); // usage

Now you can synchronize whole methods or blocks in methods according to the needs, but the Singleton itself doesn't need any synchronization.

[Dave Alvarado]

Embia, Could you provide a little more explanation? Is "INSTANCE" a coderanch, non-static member variable? If so, you also define the method "doSomething" as non-static so I'm missing the singletonness of this class.

Thanks for any clarification, - Dave

[Campbell Ritchie]

What Embla has shown you is the enumerated type. You can read about it in this section in the Java™ Tutorials.

[John de Michele]

Dave:

Enumerations are always static and final objects of their classes. So MySingleton.INSTANCE cannot be instantiated or changed.

John.

[Embla Tingeling]

Dave Alvarado wrote:Thanks for any clarification

It's a standard enum really but with one enum constant only. Under the hood, INSTANCE is a final static variable holding a reference to an instance of MySingleton. That's why the doSomething method doesn't need to be static.

This Singleton implementation is suggested in Effective Java, second edition, by Joshua Bloch, so it's nothing I've invented myself. He states,

"While this approach has yet to be widely adopted, a single-element enum type is the best way to implement a singleton".

And he's always right.

The only objection I've seen is that it somehow would violate the idea of an enum. But that seems to come mostly from people who have spent years studying the intricacies of other implementation and now is taken aback by the simplicitly and elegance of this solution. Maybe it should be called a Simpleton.

A more valid objection would be that it doesn't fulfill all criteria of a Singleton as described in the GoF design pattern book. For example it cannot be subclassed and it cannot be expanded to a "multiton". So it's maybe somewhat restricted in relation to the alternatives.

[Brian Cole]

Dave Alvarado wrote:Embia, Could you provide a little more explanation? Is "INSTANCE" a coderanch, non-static member variable? If so, you also define the method "doSomething" as non-static so I'm missing the singletonness of this class.

If you find the Enumeration semantics confusing, you could do something like this instead.

public static YouthfulDriverDatabase getInstance() {  
  return InnerClass.ydd;
}  

private static class InnerClass {
  static YouthfulDriverDatabase ydd = new YouthfulDriverDatabase();
}

The point is that ydd won't be initialized until the InnerClass class is loaded, which will the first time getInstance() is called. It works with no need for synchronized blocks or null checks.

[edit: Corrected ydd to be static, as per next post. That's what I get for coding in the forum editor without checking the code with a compiler.]

[Embla Tingeling]

Brian Cole wrote:If you find the Enumeration semantics confusing, you could do something like this instead.

The ydd variable in InnerClass needs to be made static.

[Pat Farrell]

Seriously, Singletons considered evil. Don't use one. They seem to work, and then will break in weird ways.

The best way to think about it is that a Singleton is just a global wad of data, which breaks encapsulation and makes testing hard, even if its bug free. And making them bug free is a challenge, IBM has a very nice paper on common bugs. Yet even that solution doesn't always work.

In the Web Application world, it is totally legal to have multiple class loaders in an application (JBoss, Glassfish, etc.). When this happens, you have no guarantee that your singleton is still one and only one. So if you rely upon that, you are into a world of subtle bugs that are nearly impossible to find.

Really, don't use the Singleton pattern. Use at least a factory, and better use Inversion of Control, aka Dependancy Injection.

[Embla Tingeling]

Pat Farrell wrote:Seriously, Singletons considered evil. Don't use one. They seem to work, and then will break in weird ways.

Like integers right. Just when you think everything finally is under control the little buggers suddenly overflow!

[John de Michele]

Seriously, Singletons considered evil. Don't use one. They seem to work, and then will break in weird ways.

Just like most hard-and-fast rules, this is probably overblown. Singletons are like cars: when used properly, they work just fine. When used poorly, you get Toonces the Cat. If you're loading one with multiple class loaders, then you're probably doing something even more fundamentally wrong, like mixing your layers.

John.

[Pat Farrell]

John de Michele wrote:Just like most hard-and-fast rules, this is probably overblown. Singletons are like cars: when used properly, they work just fine. When used poorly, you get Toonces the Cat.

I used to think that, and used a lot of them with the IBM article's fix for double-check bug.

But a recent Google java technology talk on testing convinced me that its really stronger. They are just global data stores, often with state. They make your code nearly impossible to properly unit test. And I believe, as do many others, that proper unit testing is critical to large scale systems that are reliable and maintainable. With a Singleton, you can't isolate classes that use it. Building mock objects is a critical part of modern testing.

As important, to me, is that it is fairly easy and painless to just write your code to not use them. And once you do, your testing is tons easier. With easier tests, you can write more tests, that explore more edge and fencepost cases.

Singletons often have state, which means your code is relying upon that state. And Singletons make your API and javadocs lie, as they don't mention the perhaps hundreds of state objects that are controlled by the singleton.

This is one more case where the Gang of Four patterns are really harmful.

[John de Michele]

Pat Farrell wrote:
I used to think that, and used a lot of them with the IBM article's fix for double-check bug.

But a recent Google java technology talk on testing convinced me that its really stronger. They are just global data stores, often with state. They make your code nearly impossible to properly unit test. And I believe, as do many others, that proper unit testing is critical to large scale systems that are reliable and maintainable. With a Singleton, you can't isolate classes that use it. Building mock objects is a critical part of modern testing.

As important, to me, is that it is fairly easy and painless to just write your code to not use them. And once you do, your testing is tons easier. With easier tests, you can write more tests, that explore more edge and fencepost cases.

Singletons often have state, which means your code is relying upon that state. And Singletons make your API and javadocs lie, as they don't mention the perhaps hundreds of state objects that are controlled by the singleton.

This is one more case where the Gang of Four patterns are really harmful.

I've seen the Google testing blog where many of these statements have been argued. As an SDET, I would agree with the sentiment that unit testing is a Good Thing, and that doing as much of it as possible is preferable. Your statements just illustrate the need to properly construct singletons, rather than throwing the baby out with the bath water.

John.

[Pat Farrell]

John de Michele wrote: Your statements just illustrate the need to properly construct singletons, rather than throwing the baby out with the bath water.

I don't see how you can "properly construct" one without having it inherently have the evil things. And further, I don't see why, since it is easy to write code without them that clearly is free of the problems, that one would argue to take the time and effort to make them "properly".

Perhaps you could enlighten me with a real world example where it is good to use a Singleton and better than using Dependancy Injection.

It is so simple to create a SingletonFactory with a "getInstance()" function, so that all of the user classes don't know about the fact that its not really a Singleton. Then all you have to do is call an application-wide initializer to initialze the SingletonFactory as you wish.

Since web applications don't have a main, you need to call the application-wide initilization code from all of the expected entry points, but a trivial

boolean isInitialized = false;

flag handles the internal implementation details.

[John de Michele]

Pat:

Why use a getInstance() at all? Use an enumeration, as was mentioned above. Since you brought up factories, I would use a singleton for a factory. Why would you ever need more than one object for that, especially if it is something heavyweight? As a real-world example, I've used a singleton in a model-based testing framework that I created, to generate node objects. The node creation was determined by a family of flyweight objects, so the factory singleton needed very little state.

John.

[Pat Farrell]

John de Michele wrote:Why use a getInstance() at all? Use an enumeration, as was mentioned above.

getInstance() is just a coding idiom. People are used to it, and know what it does from the name. Lots of standard Java libraries use them, such as Calendar.getInstance();

The enumeration usage patter is relying on a side effect of how enumerations were defined. I don't like it as a style, but it probably works at some level.

John de Michele wrote: Since you brought up factories, I would use a singleton for a factory. Why would you ever need more than one object for that, especially if it is something heavyweight? As a real-world example, I've used a singleton in a model-based testing framework that I created, to generate node objects.

There is no need to use a singleton pattern for a factory. Changing from the "use Singleton" model to "get the Singleton from the SingletonFactory" is just syntactic sugar. You still have a singleton. And you don't need it. Ever.

If you have a factory that has a default "setInstance()" entry, then you can use it as if it was a Singleton, but it is just a normal instance. Here is a complete, working, example:

 

package com.fnfbook.daemon;

import org.apache.log4j.Logger;


/**
 * The <code>MasterControlProgramFactory</code> class implements a boilerplate factory to create MasterControlProgram instances.
 * Can be used for both testing and as a factory that defaults to a reasonable creation
 * @see ConcreteMasterControlProgram
 * @see MasterControlProgram
 * 
 * @author pfarrell
 * Created on Nov 2, 2009, 12:03:22 AM
 */
public class MasterControlProgramFactory {
         /** logger instance */
protected static final Logger facLog = Logger.getLogger(MasterControlProgramFactory.class);
/** our very own MCP instance */
private static  MasterControlProgram instance;

  /**
   * sets up the factory to return a nice initance of an MCP -- note: once the instance is initialized,
   * it can not directly be changed, you must change it to null first, and then you can initialize it again. Will expliticly
   * call shutdown on instance when clearing it out.
   * @param the MCP to use
   * @throws IllegalStateException if instance was setup and an attempt is made to set it to something else.
   */
public static void setInstance( MasterControlProgram aControlProgram) {
    if (instance == null) {
        if ( aControlProgram == null)
            return;
        else if (aControlProgram != null)
            instance = aControlProgram;
    } else {
        if (aControlProgram == null) {
            instance.shutdown();
            instance = null;
        } else {
            facLog.fatal("very bad, trying to change out live MCP");
              throw new IllegalStateException(" live MasterControlProgramFactory can not be re-initialized. "
                          + "Did you forget to call MasterControlProgram.setInstance(null) ?");
        }
   }
}

  /**
   * gets the instance, if any of the MCP
   * @return the MCP instance
   */
public static  MasterControlProgram getInstance() {
    if (instance == null) {
          throw new IllegalStateException(" MasterControlProgramFactory not initialized. "
                      + "Did you forget to call MasterControlProgram.setInstance() ?");
    }
    return instance;
}
  /**
   * @return gets true if we have been initialilzed
   */
  public static boolean isInitialized() {
      return instance != null;
  }
  /**
   * creates and instantiates the usual default class implementing a MasterControlProgram. 
   * @return the created and initialized instance.
   */
  public static MasterControlProgram doDefaultSetup() {
      if (instance == null) {
          instance = new ConcreteMasterControlProgram();
          instance.ensureRunning();
      }
      return getInstance();
  }
}
   

In your JUnit tests, you can call the setInstance() with any instance you like, real or mock.

In your working code, you let someone else call the setInstance() within a "isInitialized()" block.

It looks like a Singleton to all your code, but it is not, its a ton easier to test, and it ends up being a ton more reliable.