Hi,
I used quite a few Singletons in my own code, and recently found I’'d been using them incorrectly. The pattern as used by XmppServer.java also seems incorrect:
public class XmppServer
{
private static final thisObject;
public XmppServer()
{
…
thisObject = this;
…
}
public static XmppServer getInstance()
{
return thisObject;
}
…
}
/code
This approach seems flawed, mainly because you’'re making a partially constructed object visible to the outside, which should never happen. Rather than assigning “thisObject=this” in the constructor, why not declare thisObject to be: private static final XmppServer thisObject = new XmppServer() ? This gets executed before any non-static calls, which should make this Thread safe.
Might this still give multi-threaded issues, where the compiler makes a call to getInstance() (from other objects’’ constructors which might also be statically initialized?) before thisObject is actually set? Meaning you’'d get a nullpointer at runtime, which would not be nice. Either way, the current implementation seems wrong.
Thoughts?
–
Frank
what about
private static final thisObject = null;
public static XmppServer getInstance()
{
synchronized(thisObject) {
if(thisObject == null)
{
thisObject = new XmppServer();
}
}
return thisObject;
}
/code
(not really tested of course)
thisObject cannot be final here I guess (where’'s that edit-link gone in this forum?)
I missed the edit button myself…
Anyway, your solution would work, and is a common implementation. But it would introduce synchronization at every single call to the method. Typically you want to avoid that if at all possible.
You can use a double-checked lock, where you do the following in getInstance():
if(thisObject == null)
{
synchronized(thisObject)
{
if(thisObject = null)
{…}
}
}
else
{
return thisObject
}
/code
But this is not thread safe.
–
Frank
(read up on this stuff just this morning)
Your code is reentrant as long as the object can never become null after being non-null (which is the case here)!
btw, you forgot one = in the inner if-clause
Note that the double-check optimization is broken in Java until/unless they fix the JVM memory model. See:
http://www-128.ibm.com/developerworks/java/library/j-dcl.html
for a detailed explanation.
… WkH
Indeed. Anyway, the current XmppServer.getInstance() is not thread-safe, nor even correct due to publishing ‘‘this’’ from within the constructor, and should be updated with some other implementation of the singleton pattern. It’‘s possible this occurs elsewhere in the code as well. It’‘s a relatively minor issue, but important for program correctness. I figured I’'d mention it.