views:

2096

answers:

11

I'm trying to track down an issue in our system and the following code worries me. The following occurs in our doPost() method in the primary servlet (names have been changed to protect the guilty):

...
if(Single.getInstance().firstTime()){
   doPreperations();
}
normalResponse();
...

The singleton 'Single' looks like this:

private static Single theInstance = new Single();

private Single() {
...load properties...
}

public static Single getInstance() {
    return theInstance;
}

With the way this is set to use a static initializer instead of checking for a null theInstance in the getInstance() method, could this get rebuilt over and over again?

PS - We're running WebSphere 6 with the App on Java 1.4

+11  A: 

No it won't get built over and over again. It's static, so it'll only be constructed once, right when the class is touched for the first time by the Classloader.

Epaga
+1  A: 

No - the static initialization of the instance will only ever be done once. Two things to consider:

  • This is not thread-safe (the instance is not "published" to main memory)
  • Your firstTime method is probably called multiple times, unless properly synchronized
oxbow_lakes
What do you mean by "This is not thread-safe (the instance is not "published" to main memory)"?
Dan Dyer
Within a JVM there is both "main" memory (shared between threads) and "thread-local" memory. Because the instance field is neither final nor volatile, changes to the variable made in one thread are not necessarily visible in another. The other way to ensure visiblility is the synchronized keyword.
oxbow_lakes
+12  A: 

I found this on Sun's site:

Multiple Singletons Simultaneously Loaded by Different Class Loaders

When two class loaders load a class, you actually have two copies of the class, and each one can have its own Singleton instance. That is particularly relevant in servlets running in certain servlet engines (iPlanet for example), where each servlet by default uses its own class loader. Two different servlets accessing a joint Singleton will, in fact, get two different objects.

Multiple class loaders occur more commonly than you might think. When browsers load classes from the network for use by applets, they use a separate class loader for each server address. Similarly, Jini and RMI systems may use a separate class loader for the different code bases from which they download class files. If your own system uses custom class loaders, all the same issues may arise.

If loaded by different class loaders, two classes with the same name, even the same package name, are treated as distinct -- even if, in fact, they are byte-for-byte the same class. The different class loaders represent different namespaces that distinguish classes (even though the classes' names are the same), so that the two MySingleton classes are in fact distinct. (See "Class Loaders as a Namespace Mechanism" in Resources.) Since two Singleton objects belong to two classes of the same name, it will appear at first glance that there are two Singleton objects of the same class.

Citation.

In addition to the above issue, if firstTime() is not synchronized, you could have threading issues there as well.

Chris Marasti-Georg
Which would affect either potential implementation.
rice
Indeed... this is a nice answer to a completely different question from what was asked. Therefore, I'm not sure why its gotten voted up so much?
jsight
Well actually, he's asking if it's possible for his singleton to get rebuilt over and over again, and the answer is yes - in some containers that use multiple class loaders, it could.
Chris Marasti-Georg
Re threading issues: synchronizing firstTime() won't help at all; you must synchronize the code which firstTime() protects!
Aaron Digulla
This is a bit misleading, while there *are* many cases where there are multiple classloaders, in most circumstances a classloader would not load a class that had already been loaded by a parent. So while you shouldn't rely on a class being unique, it would be pretty rare for this effect to defeat the singleton. Far more common would be the case of a cluster of VMs, where each would happily grab their own singleton.
CurtainDog
+1  A: 

In theory it will be built only once. However, this pattern breaks in various application servers, where you can get multiple instances of 'singleton' classes (since they are not thread-safe).

Also, the singleton pattern has been critized a lot. See for instance Singleton I love you, but you're bringing me down

Einar
Singletons can be thread-safe. The example in the question is thread-safe. If you get multiple instances in an application server it is probably down to having multiple class-loaders.
Dan Dyer
+2  A: 

The only thing I would change about that Singleton implementation (other than not using a Singleton at all) is to make the instance field final. The static field will be initialised once, on class load. Since classes are loaded lazily, you effectively get lazy instantiation for free.

Of course, if it's loaded from separate class loaders you get multiple "singletons", but that's a limitation of every singleton idiom in Java.

EDIT: The firstTime() and doPreparations() bits do look suspect though. Can't they be moved into the constructor of the singleton instance?

Dan Dyer
+5  A: 

As others have mentioned, the static initializer will only be run once per classloader.

One thing I would take a look at is the firstTime() method - why can't the work in doPreparations() be handled within the singleton itself?

Sounds like a nasty set of dependencies.

matt b
Sounds like someone doesn't understand Servlet development, actually.
Spencer K
A: 

This will get only loaded once when the class is loaded by the classloader. This example provides a better Singleton implementation however, it's as lazy-loaded as possible and thread-safe. Moreover, it works in all known versions of Java. This solution is the most portable across different Java compilers and virtual machines.


public class Single {

private static class SingleHolder {
   private static final Single INSTANCE = new Single();
}

private Single() {
...load properties...
}

public static Single getInstance() {
    return SingleHolder.INSTANCE;
}

}

The inner class is referenced no earlier (and therefore loaded no earlier by the class loader) than the moment that getInstance() is called. Thus, this solution is thread-safe without requiring special language constructs (i.e. volatile and/or synchronized).

nkr1pt
Lazy loading is known to be broken.
Martin Spamer
Can you provide a source stating this?IODH utilizes lazy class initialization. The JVM won't execute a class's static initializer until you actually touch something in the class. This applies to static nested classes, too, according to the Java Language Specification
nkr1pt
Here the SingleHoler will not initialize the INSTANCE until somebody calls getInstance(). And it is thread safe since it's initialized in the static block.
Cem Catikkas
@Martin. It's not broken if implemented correctly. I would use the simpler approach, without the holder, but this implementation is fine.
Dan Dyer
@Martin nkr1pt's code doesn't use double-checked locking. Are you saying there is something wrong with this code? You are conflating double-checked locking and lazy initialisation in your comments. The latter does not imply the former. DCL *without* volatile is broken. With volatile it is safe.
Dan Dyer
+4  A: 

There is absolutely no difference between using a static initializer and lazy initialization. In fact it's far easier to mess up the lazy initialization, which also enforces synchronization. The JVM guarantees that the static initializer is always run before the class is accessed and it will happen once and only once.

That said JVM does not guarantee that your class will be loaded only once. However even if it is loaded more than once, your web application will still see only the relevant singleton, as it will be loaded either in the web application classloader or its parent. If you have several web application deployed, then firstTime() will be called once for each application.

The most apparent things to check is that firstTime() needs to be synchronized and that the firstTime flag is set before exiting that method.

Jevgeni Kabanov
Lazy initialisation is KNOWn to be broken.
Martin Spamer
Unsynchronized one is. Class initialization is always synchronized, so it's not a problem. The link you are giving is contradicting your words :)
Jevgeni Kabanov
Peter Haggar : the double-checked locking idiom is still broken under the new memory model. http://www.ibm.com/developerworks/java/library/j-dcl.html
Martin Spamer
Brian Goetz: Under the new memory model, this "fix" to double-checked locking renders the idiom thread-safe. http://www.ibm.com/developerworks/library/j-jtp03304/
Dan Dyer
Don't see how any of this is relevant to the problem. Initializing in static initializer is perfectly thread-safe. JVM locks the class when doing the initialization, so concurrent accesses have to wait.
Jevgeni Kabanov
A: 

please note that the original example is indeed not correct. no one has pointed out that the classloader reference loading correctness is guaranteed only if the reference is final!

james
Can anyone confirm this?
Adam
+2  A: 

No, It won't create multiple copies of 'Single'. ( Classloader issue will be visited later )

The implementation you outlined is described as 'Eager Initialization' by in Briant Goetz's book - 'Java Concurrency in Practice'.

public class Single
{
    private static Single theInstance = new Single();

    private Single() 
    { 
        // load properties
    }

    public static Single getInstance() 
    {
        return theInstance;
    }
}

However, the code is not you wanted. Your code is trying to perform lazy-initialization after the instance is created. This requires all the client library to perform 'firstTime()/doPreparation()' before using it. You are going to rely on the client to do right thing which make the code very fragile.

You can modify the code as the following so there won't be any duplicate code.

public class Single
{
    private static Single theInstance = new Single();

    private Single() 
    { 
        // load properties
    }

    public static Single getInstance() 
    {   
        // check for initialization of theInstance
        if ( theInstance.firstTime() )
           theInstance.doPreparation();

        return theInstance;
    }
}

Unfortunately, this is a poor implementation of lazy initialization and this will not work in concurrent environment ( like J2EE container ).

There are many articles written about Singleton initialization, specifically on memory model. JSR 133 addressed many weakness in Java memory model in Java 1.5 and 1.6.

With Java 1.5 & 1.6, you have several choices and they are mentioned in the book 'Effective Java' by Joshua Bloch.

  1. Eager Initialziation, like the above [EJ Item 3]
  2. Lazy Initalization Holder Class Idiom [EJ Item 71]
  3. Enum Type [EJ Item 3]
  4. Double Checked Locking with 'volatile' static field [EJ Item 71]

Solution 3 and 4 will only work in Java 1.5 and above. So the best solution would be #2.

Here is the psuedo-implementation.

public class Single
{
    private static class SingleHolder
    {
        public static Single theInstance = new Single();
    }

    private Single() 
    { 
        // load properties
        doPreparation();
    }

    public static Single getInstance() 
    {
        return SingleHolder.theInstance;
    }
}

Notice that 'doPreparation()' is inside of the constructor so you are guarantee to get the properly initialized instance. Also, you are piggying back on JVM's lazy class loading and do not need any synchronization 'getInstance()'.

One thing you noticed that static field theInstance is not 'final'. The example on Java Concurrency does not have 'final' but EJ does. Maybe James's can add more color to his answer on 'classloader' and requirement of 'final' to guarantee correctness,

Having said that, there are a side-effect that with using 'static final'. Java compiler is very aggressive when it sees 'static final' and tries to inline it as much as possible. This is mentioned on a blog posting by Jeremy Manson.

Here is a simple example.

file: A.java

public class A
{
    final static String word = "Hello World";
}

file: B.java

public class B
{
    public static void main(String[] args) {
        System.out.println(A.word);
    }
}

After you compile both A.java and B.java, you change A.java to following.

file: A.java

public class A
{
    final static String word = "Goodbye World";
}

You recompile 'A.java' and rerun B.class. The output you would get is

Hello World

As for the classloader issue, the answer is yes, you can have more than one instance of Singleton in multiple classloaders. You can find more information on wikipedia. There is also a specific article on Websphere.

mjlee
A: 

It's not mandatory for the single instance to be final (it's not a good idea at all indeed, because this will avoid you to switch it's behaviour using other patterns).

In the code below you can see how it gets instantiated only once (first time you call the constructor)

package date;

import java.util.Date;

public class USDateFactory implements DateFactory { private static USDateFactory usdatefactory = null;

private USDateFactory () { }

public static USDateFactory getUsdatefactory() {
    if(usdatefactory==null) {
        usdatefactory = new USDateFactory();
    }
    return usdatefactory;
}

public String getTextDate (Date date) {
    return null;
}

public NumericalDate getNumericalDate (Date date) {
    return null;
}

}