Singleton in multithreads

[jee soa]

Hi:
I need to have a singleton in multithreaded environment.
I have the following code, is it safe.

public class Singleton
{
 
    private static Singleton instance = null;
    [B]private static XYZXlass xyz = null;[/B]
 
    private Singleton()
    {
    }
   
    public static Singleton getInstance() throws Exception {
        synchronized(Singleton.class) {          
        if(instance == null)
        instance = new Singleton();        
        [B]if(xyz == null)
        xyz = new XYZXlass("abc","efg");[/B]
        }
    return instance;
    }
   [B]
    public XYZXlass getXyz()
    {
        return xyz;
    }
    [/B]
}

[Amit M Tank]

Yes it is.

[Jim Yingst]

Ummmm... what do you think happens if someone just calls getXyz(), with no call to getInstance()? Is that OK?

[Amit M Tank]

How can he call getXyz() without calling the getInstance first?

[Jim Yingst]

Ah, yes. I overlooked that it's an instance method. In that case, the same effect could be achieved more simply with

public class Singleton {
 
    private static Singleton instance = null;
    private final XYZXlass xyz = new XYZXlass("abc","efg");
 
    private Singleton() {}
   
    public static synchronized Singleton getInstance() throws Exception {
       if(instance == null)
            instance = new Singleton();  
        return instance;
    }
   
    public XYZXlass getXyz() { return xyz; }
}

[jee soa]

thank you Jim and Amit for the replies.

Is "private final XYZXlass xyz = new XYZXlass("abc","efg");" it thread safe. Along with Singleton class I want only one instantiation for XVZXClass for all the threads and thread safe.

Please let me know.

[Jim Yingst]

Yes, when I said it had the same effect, what I meant was that it had the same effect. Including thread safety. Since there's only one instance of the Singleton, its xyz field only gets initialized once.

[Peter Chase]

It's safe, but it's bizarre. Why have a singleton and then have further static data?

[jee soa]

I can not use "private final XYZXlass xyz = new XYZXlass("abc","efg");" as the code will not compile due to not handling compile time error.

so what is the best way to do it.

    
public class Singleton
{
 
    private static Singleton instance = null;
    private XYZXlass xyz = null;
    or 
    private static XYZXlass xyz = null;
 
 
    private Singleton()
    {
    }
   
    public static Singleton getInstance() throws Exception {
        synchronized(Singleton.class) {          
        if(instance == null)
        instance = new Singleton();        
        if(xyz == null)
        xyz = new XYZXlass("abc","efg");
        }
    return instance;
    }
   
    public XYZXlass getXyz()
    {
        return xyz;
    }
    
}

Is it a good idea to move the "xyz = new XYZXlass("abc","efg");" code inside constructor.? Will it guarantee thread safety.?

Thanks for all for the comments and suggestions.

[Jim Yingst]

[jee soa]: Is it a good idea to move the "xyz = new XYZXlass("abc","efg");" code inside constructor.? Will it guarantee thread safety.?

Only if you keep xyz final, as I showed. Otherwise there are subtle threading issues.

public class Singleton {
 
    private static Singleton instance = null;
    private final XYZXlass xyz;
 
    private Singleton() throws Exception { 
        xyz = new XYZXlass("abc","efg"); 
    }
   
    public static synchronized Singleton getInstance() throws Exception {
       if(instance == null)
            instance = new Singleton();  
        return instance;
    }
   
    public XYZXlass getXyz() { return xyz; }
}

[jee soa]

so the following code is fine.?

public class Singleton { 
    private static Singleton instance = null;
    private XYZXlass xyz; 
    private Singleton() throws Exception { xyz = new XYZXlass("abc","efg");}    
       public static synchronized Singleton getInstance() throws Exception {
       if(instance == null) instance = new Singleton();  
        return instance;
    }   
    public XYZXlass getXyz() { return xyz; }
}

[Jim Yingst]

No.

Originally posted by Jim Yingst:
Only if you keep xyz final, as I showed. Otherwise there are subtle threading issues.

If you remove the final, and the variable xyz is initialized in non-static code, then it's not thread-safe. That's why I said you needed to use final. That's why I showed the code with final. If you remove the final, the code is not thread safe. THEREFORE, DO NOT REMOVE THE FINAL.
[ July 03, 2007: Message edited by: Jim Yingst ]

[Adeel Ansari]

Originally posted by Jim Yingst:
No. If you remove the final, and the variable xyz is initialized in non-static code, then it's not thread-safe. That's why I said you needed to use final. That's why I showed the code with final.

I don't know if I am missing some point. But do you think that since it is private and have no setter method even then it needs final?

[Peter Chase]

Aside from the issues of this particular case, any instance field that gets set during construction and is not supposed to change afterwards should be declared "final". This has a number of advantages: -

Safer: Compiler will detect any later inappropriate modifications to the code, which try to change the field

Self-documenting: declaring it "final" makes it clear that the value will not and cannot change

Enables optimisation: bytecode and/or HotSpot compiler may make use of the knowledge that the value cannot change, to enable optimisations to make the compiled code faster and/or smaller

[Jim Yingst]

[Adeel]: I don't know if I am missing some point. But do you think that since it is private and have no setter method even then it needs final?

Yes, absolutely. The compiler isn't going to check all the other methods of a class to see if a given field is changed after instatiation or not. It just checks whether or not the field is declared final or not. Much simpler. See JLS3 17.5 for more info. Specifically, look at the discussion of FinalFieldExample and the difference between FinalFieldExample.x and FinalFieldExample.y.
[ July 04, 2007: Message edited by: Jim Yingst ]

[Adeel Ansari]

Originally posted by Peter Chase:
This has a number of advantages: -

Safer: Compiler will detect any later inappropriate modifications to the code, which try to change the field

Self-documenting: declaring it "final" makes it clear that the value will not and cannot change

Enables optimisation: bytecode and/or HotSpot compiler may make use of the knowledge that the value cannot change, to enable optimisations to make the compiled code faster and/or smaller

Yes, the last two I understand and practice the same for the same reason. I was just curious about the first one as I already told that what thing I didn't understand.
Thanks to you.

Thanks Jim for the link. That makes it clear.

[jee soa]

Thank You for all the inputs and suggestions.

[Peter Chase]

Originally posted by Adeel Ansari:

I was just curious about the first one as I already told that what thing I didn't understand.

When writing any real piece of software, you should consider how it will be maintained and improved over time. Your software should be easy to understand and easy to modify correctly.

The word "correctly" is important above.

Say you have a certain instance field is set at construction and which it doesn't make sense to change after construction. While you're writing your code currently, you'll probably remember this. But if you, or someone else, comes to the code later on, they might not know and they might write something that changes that field after construction. This could break other parts of the code that rely on it not changing.

If you declare it "final", the compiler will barf when someone tries to compile code to modify the field after construction. This will flag up to the maintaining developer that the field isn't supposed to change. They'll hopefully then adopt an alternative approach.

Of course, if the maintainer is a complete cowboy, they may just delete the "final" and carry on...