Singleton & multithread env.

[Mihai Radulescu]

Hallo,
I have a singleton used in multithread environment see the code :

  
 
public class Registry {
 
    /**
     * The <code>Registry<code> is a singleton and here
     * it holds its unique instance.
     */
    private static Registry registry;
 
    /**
     * Here are the registerd values stored.
     */
    private final WeakHashMap values;
 
    /**
     * Gets a <code>Registry<code> instance.
     *
     * @return Registry
     */
    public static Registry getInstance() {
        if(registry == null) {
            synchronized (Registry.class) {
                if(registry == null) {
                    registry = new Registry();
                }
            }
        }
 
        return registry;
    }
 
    /**
     * Builds the registry.
     */
    private Registry() {
        values = new WeakHashMap();
    }
 
    
    public synchronized Object register(String key, Object value) {
        return values.put(key, value);
    }
 
    public synchronized Object request(String key) {
        return values.get(key);
    }
 
    public synchronized Object removeKey(String key) {
        return values.remove(key);
    }
 
    public synchronized void close() {
        values.clear();
    }
}

This is a simple registry mechanism.
The user can obtain only one instance (the private static Registry registry) via the getInstance method - here I use the D Schmidt's "double checked loocking" so if two (or more) threads try to get an instance only one is created.
For the rest of methods I synchronized on this (the unique registry instace)
so the rest is also safe.

So what do you all think ?

Regards,Mihai

[Arto Pastinen]

Hi!

Instead synchronizing register, request removeKey and close methods, synchronize values object.

example:

public void close() {
synchronized(values) {
values.clear();
}
}

and other very little performace issue is that you don't have to test is registry null in getInstance.

I usually make singleton class this way:

public class Foo {
private static final Foo instance = new Foo();

private Foo() {}

public static Foo getInstance() { return instance; }
}

[Ed Wallen]

Do not use double-check locking for your Singleton getInstance() method. The bottom line is that double-checked locking, in whatever form, should not be used because you cannot guarantee that it will work on any JVM implementation. JSR-133 is addressing issues regarding the memory model, however, double-checked locking will not be supported by the new memory model.

Read this article:

Double-check locking and the Singleton pattern

-Ed

[Stan James]

Double checked locking is pretty well proven broken in multi-threaded and particularly multi-processor applications. It's much simpler and safer to just say:

private static final Singleton singleton = new Singleton();

Ed beat met to this message! I'm back to edit out a link I had that accidentally included smileys or something. I just googled for "java double checked locking broken" and found scads of articles.

Hope that helps!
[ March 15, 2005: Message edited by: Stan James ]

[Alan Mehio]

Many thanks to the link it is very nice article to be aware of

[Mr. C Lamont Gilbert]

Originally posted by Ed Wallen:
Do not use double-check locking for your Singleton getInstance() method. The bottom line is that double-checked locking, in whatever form, should not be used because you cannot guarantee that it will work on any JVM implementation...

-Ed

It is only guaranteed to work if the machine designers say so. This is not just for virtual machines but it suffers the same issue running directly on the CPU. So unless your AMD Athlon manual has information supporting this technique, it may fail even with assembly/c/c++.

[Mihai Radulescu]

Ok , I also have some news
I just read the "Head first design patterns" and on the singleton pattern in multithread environment I found :

.........in the java version 1.4 and earlier , amny JVMs contains implementations of the volatile keyword that allow improper synchronization for the double-checked locking.

with othe words

 
if(jvm.getVersion() > 1.5.0 ) {
    use double-checked locking
} else {
    switch (yourContext) {
       case singleton eat resources:
           lazy init with synchronize on getInstance
           break;
       case :
           eager instantion
       break;
    }    
}

[Mr. C Lamont Gilbert]

Yes I heard there would be work towards making this work in 1.5. I also heard there was work towards making a lock check very quick. Seems a waste to do both.

We just don't need double checked locking. Unless you plan to hit this method LOTS of times, and even then its really not an issue.

If they do claim to have made it work on 1.5, i predict it will only be in certain JVMs and as a result nobody will really be able to use it.

[David Harkness]

Warning: Micro-optimization area ahead. Hard hat must be worn at all times.

If you expect far more calls to request() than the other methods and the size of the map isn't "too large," you can avoid synchronization for request() using copy-on-write. When the Registry needs to modify the values map, it modifies a copy instead and then swaps it in for the real one. Thus, only the modifying methods need to be synchronized.

public synchronized Object register(String key, Object value) {
    WeakHashMap newValues = (WeakHashMap) values.clone();
    Object replaced = newValues.put(key, value);
    values = newValues;
    return replaced;
}
 
public Object request(String key) {
    return values.get(key);
}
 
public synchronized Object removeKey(String key) {
    WeakHashMap newValues = (WeakHashMap) values.clone();
    Object removed = newValues.remove(key);
    values = newValues;
    return removed;
}
 
public synchronized void close() {
    WeakHashMap newValues = new WeakHashWeakHashMap();
    values = newValues;
}

[ Updated after re-reading JLS chapter 17 which states that only long and double suffer from non-atomicity. ]

The only issue is that the JVM is not required to perform the store/write actions to main memory. Thus, it's possible that the map could be swapped out with a new one, but only the thread that performed the swap sees the effect.

Making the variable volatile solves this but is only slightly faster than synchronizing. However, it still allows concurrent access which synchronization does not, which was the point all along (for me, anyway ).
[ April 11, 2005: Message edited by: David Harkness ]