Multi-threaded safe singleton class question

  • Thread starter Thread starter John Lee
  • Start date Start date
J

John Lee

Hi,

I like the C# implemention of singleton class presented at MSDN. My question
is that is it correct or right thing to do to modify that class to make the
class will return the same instance if for the same parameter when calling
GetInstance?

public static Singleton GetInstance(string parameter)
{
//using a hashtable or some other type to hold the instance - parameter
will be the key
//if the key exists and the instance is not null then return the
instance, otherwise create new instance and then add to hashtable
}

Thanks a lot!
John
 
We're doing exactly this for many of our "singleton" classes, and it
works just fine. Whether it's thread-safe is another question entirely.
I'll leave that to Jon Skeet or someone else more familiar with
threading.
 
John,

In this case, it isn't really a singleton, in the sense that you have
more than one Singleton instance.

In order to guarantee thread safety, you should pass the hashtable to
the static Synchronized method on the Hashtable class. This will pass a
wrapper to you which will provide thread safety when performing operations
(read and write) on the Hashtable.

Hope this helps.
 
John Lee said:
I like the C# implemention of singleton class presented at MSDN. My question
is that is it correct or right thing to do to modify that class to make the
class will return the same instance if for the same parameter when calling
GetInstance?

public static Singleton GetInstance(string parameter)
{
//using a hashtable or some other type to hold the instance - parameter
will be the key
//if the key exists and the instance is not null then return the
instance, otherwise create new instance and then add to hashtable
}
Click to expand...

As Nicholas said, that's not really a singleton - it's a factory
pattern implementation.

For thread safety, you should take out a lock:

static object mapLock = new object();

public static Singleton GetInstance (string parameter)
{
lock (mapLock)
{
// Check for existence, create new instance etc and return it
}
}

Don't try double-checked locking - unless you're *really, really*
confident that you're doing it right, and have got an expert to
validate it, preferrably by talking with even more experts, you may
well have a problem.
 
Thanks very much!!!

Is this a thread-safe implementation?

=============================================
private static Hashtable htAzApps;

public static AzManStore GetInstance(string applicationName)
{
lock (htAzApps.SyncRoot)
{
if (htAzApps == null)
{
htAzApps = new Hashtable();
instance = new AzManStore(applicationName);
}
else
{
if (htAzApps.ContainsKey(applicationName))
instance = (AzManStore)htAzApps[applicationName];
else
instance = new AzManStore(applicationName);
}
}
return instance;
}
=================================================
 
John,
lock (htAzApps.SyncRoot)
{
if (htAzApps == null)
Click to expand...

This will clearly not work. You have to verify that htAzApps != null
before the lock, or you'll get a NullReferenceException accessing
htAzApps.SyncRoot.

You can do what Jon showed you and lock on a separate object.



Mattias
 
John Lee said:
Thanks very much!!!

Is this a thread-safe implementation?

=============================================
private static Hashtable htAzApps;

public static AzManStore GetInstance(string applicationName)
{
lock (htAzApps.SyncRoot)
{
if (htAzApps == null)
{
htAzApps = new Hashtable();
instance = new AzManStore(applicationName);
}
else
{
if (htAzApps.ContainsKey(applicationName))
instance = (AzManStore)htAzApps[applicationName];
else
instance = new AzManStore(applicationName);
}
}
return instance;
}
=================================================
Click to expand...

Not quite - look at what you're trying to lock on, and then think about
the first time through... I'd suggest having a static variable with an
object created just for locking.

(Note that you're also never saving the instance back into the hash
table, which I assume you want to be.)
 
I normally do not use the singleton pattern, in the GOF sense. But
instead a "canonical object", providing the same feature: a single place
in the code where you can obtain your singleton.

The canonical object has the benefit of not requiring static's, and thus
you can easily replace the canonical object with (for example) an other
implementation that does debug-logging.

Example implementation:

class AzManRegistry {
public static AzManRegistry Global = new Applications();
IDictionary existing;
public AzManRegistry() { existing = new Hashtable(); }
public AzManStore Instance(string applicationName) {
// Registry is monotonic, so we can use any existing
// entry if there is one... without synchronization
if ( existing.ContainsKey(applicationName) )
return existing[applicationName];
lock (existing.SyncRoot) {
// we need to recheck existance inside the critical region
if ( !existing.ContainsKey(applicationName) )
existing[applicationName] = new AzManStore(applicationName);
return (AzManStore)existing[applicationName];
}
}
}

some other place:

AzManRegistry.Global.Instance("foo");
 
Yes, you are right!

Does the following code work?

private static Hashtable htAzApps = new Hashtable();
public static AzManStore GetInstance(string applicationName)
{
lock (htAzApps.SyncRoot)
{
if (htAzApps.ContainsKey(applicationName))
instance = (AzManStore)htAzApps[applicationName];
else
{
instance = new AzManStore(applicationName);
htAzApps.Add(applicationName, instance);
}
}
return instance;
}

Thanks very much!
John
 
Helge Jensen said:
I normally do not use the singleton pattern, in the GOF sense. But
instead a "canonical object", providing the same feature: a single place
in the code where you can obtain your singleton.

The canonical object has the benefit of not requiring static's, and thus
you can easily replace the canonical object with (for example) an other
implementation that does debug-logging.

Example implementation:

class AzManRegistry {
public static AzManRegistry Global = new Applications();
IDictionary existing;
public AzManRegistry() { existing = new Hashtable(); }
public AzManStore Instance(string applicationName) {
// Registry is monotonic, so we can use any existing
// entry if there is one... without synchronization
if ( existing.ContainsKey(applicationName) )
return existing[applicationName];
lock (existing.SyncRoot) {
// we need to recheck existance inside the critical region
if ( !existing.ContainsKey(applicationName) )
existing[applicationName] = new AzManStore(applicationName);
return (AzManStore)existing[applicationName];
}
}
}
Click to expand...

You're still relying on double-checked locking, which doesn't work in
..NET without extra volatility. In the above, I don't believe there's
anything to stop the JIT from reordering things such that the new entry
in the hashtable becomes visible before all the writes in the
AzManStore constructor have finished executing.
 
Jon said:
Helge Jensen <helge.jensen@slog.dk> wrote:
Click to expand...
You're still relying on double-checked locking, which doesn't work in
.NET without extra volatility. In the above, I don't believe there's
anything to stop the JIT from reordering things such that the new entry
in the hashtable becomes visible before all the writes in the
AzManStore constructor have finished executing.
Click to expand...

BTW: the reason I posted the solution was to present the
"canonical-object" as opposed to the "singleton".

Secondly, I'm very interested in the actual problem in the code. The
code is almost identical to production-code I have written. So I am very
interested in knowing exactly what is wrong with it!

I'm not really _sure_ what's the problem. Is it the check outside the lock?
// Registry is monotonic, so we can use any existing
// entry if there is one... without synchronization
if ( existing.ContainsKey(applicationName) )
return existing[applicationName];
Click to expand...

I can see the issue, if the impl. of "existing[object item] { set }",
changes the representation of "existing" in a way that makes a
concurrent "existing[object item] { get }" either: diverge, throw, or
(very inlikely) return true.

Is this the problem? I don't think it's a problem with
System.Collections.Hashtable though. (It's not a problem that would
occur in my own Hashtable implementation, due to care in the way the
underlying datastructure is updated :)

Or is it that the AzManStore constructor inside the lock can be moved
outside the lock and executed before the lock, effectively doing:

AzManStore _moved_outside_lock = new AzManStore(applicationName);
if ( existing.ContainsKey(applicationName) )
return existing[applicationName];
lock (existing.SyncRoot) {
// we need to recheck existance inside the critical region
if ( !existing.ContainsKey(applicationName) )
existing[applicationName] = _moved_outside_lock;
return (AzManStore)existing[applicationName];
}
Click to expand...

Which would seem pretty odd to me...

Best guess (from your explanation as I read it, I may be wrong) is that
the AzManStore-construtor would not have been completely evaluated
before invoking "existing[applicationName] { set }" on the new object.
But in that case locking wouldn't help at all, at least from what I can
reason.

or is it something even more exotic?

I would really appreciate an exact description, or reference to an
article, which would make it possible for me to understand and avoid
this problem .
 
Helge Jensen said:
BTW: the reason I posted the solution was to present the
"canonical-object" as opposed to the "singleton".
Click to expand...

But if you're going to post sample code and suggest that people use it,
you should make sure it's thread-safe - certainly in a question about
thread safety!
Secondly, I'm very interested in the actual problem in the code. The
code is almost identical to production-code I have written. So I am very
interested in knowing exactly what is wrong with it!

I'm not really _sure_ what's the problem. Is it the check outside the
lock?
Click to expand...

Yes. Read up on double-checked locking and why it's broken in Java and
..NET. A simple google search should be fine.
// Registry is monotonic, so we can use any existing
// entry if there is one... without synchronization
if ( existing.ContainsKey(applicationName) )
return existing[applicationName];
Click to expand...

I can see the issue, if the impl. of "existing[object item] { set }",
changes the representation of "existing" in a way that makes a
concurrent "existing[object item] { get }" either: diverge, throw, or
(very inlikely) return true.

Is this the problem? I don't think it's a problem with
System.Collections.Hashtable though. (It's not a problem that would
occur in my own Hashtable implementation, due to care in the way the
underlying datastructure is updated :)

Or is it that the AzManStore constructor inside the lock can be moved
outside the lock and executed before the lock, effectively doing:
Click to expand...

To put it very briefly (as my dinner is waiting) - writes within the
constructor may be made visible to a reading thread *after* the write
into the hashtable. That means another thread could start using the
object before it had really been properly initialised.
 
Back
Top