Thread safety ??

This doubt is regarding synchronisation question in Singleton pattern code
of C#

I had created a class as

public sealed class SecuriteManager
{
private static volatile SecurityManager instance;
private static object syncRoot = new Object();

private SecurityManager() { }

public static SecurityManager GetInstance
{
get{
if(null == instance){
lock(syncRoot){
if (instance == null)
instance = new SecurityManager();
}
}
return instance;
}
}
Any reason to use this complicated pattern rather than a simple one as
specified on

http://www.pobox.com/~skeet/csharp/singleton.html

?
public bool IsAllowed(string UserName)
{
//A very long process here.
//For example access to a webservice which might take 3 seconds.
//HttpContext.Current.Session[UserName] = returnValueFromWebservice;
}
}
Now when 2 users access IsAllowed at the same time, is the process thread
safe ?

This doubt is regarding synchronisation question in Singleton pattern code
of C#

I had created a class as

public sealed class SecuriteManager
{
private static volatile SecurityManager instance;
private static object syncRoot = new Object();

private SecurityManager() { }

public static SecurityManager GetInstance
{
get{
if(null == instance){
lock(syncRoot){
if (instance == null)
instance = new SecurityManager();
}
}
return instance;
}
}

public bool IsAllowed(string UserName)
{
//A very long process here.
//For example access to a webservice which might take 3 seconds.
//HttpContext.Current.Session[UserName] = returnValueFromWebservice; }
}
Now when 2 users access IsAllowed at the same time, is the process thread
safe ?
Or should I use lock for each call in my function as

public bool IsAllowed(string UserName)
{
lock(syncRoot)
{
//A very long process here.
//For example access to a webservice which might take 3 seconds.
//HttpContext.Current.Session[UserName] = returnValueFromWebservice; }
}

Please suggest.

This doubt is regarding synchronisation question in Singleton pattern code
of C#

I had created a class as

public sealed class SecuriteManager
{
private static volatile SecurityManager instance;
private static object syncRoot = new Object();

private SecurityManager() { }

public static SecurityManager GetInstance
{
get{
if(null == instance){
lock(syncRoot){
if (instance == null)
instance = new SecurityManager();
}
}
return instance;
}
}

public bool IsAllowed(string UserName)
{
//A very long process here.
//For example access to a webservice which might take 3 seconds.
//HttpContext.Current.Session[UserName] = returnValueFromWebservice; }
}
Now when 2 users access IsAllowed at the same time, is the process thread
safe ?
Or should I use lock for each call in my function as

public bool IsAllowed(string UserName)
{
lock(syncRoot)
{
//A very long process here.
//For example access to a webservice which might take 3 seconds.
//HttpContext.Current.Session[UserName] = returnValueFromWebservice; }
}

Please suggest.

>private static volatile SecurityManager instance;

In addition to the other comments, should you wish to continue with a
singleton class then the volatile keyword is not needed.

Yes it is - otherwise the double-checked locking he's got isn't thread-

Phil Jenson <ph**@REMOVETHIS.jenson.co.uk> wrote:

>private static volatile SecurityManager instance;

In addition to the other comments, should you wish to continue with a
singleton class then the volatile keyword is not needed.

Yes it is - otherwise the double-checked locking he's got isn't thread-
safe. It's not the nicest way of achieving thread-safety in the first
place, but at least it *is* safe at the moment.

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet
If replying to the group, please do not mail me too

Didn't we conclude before that even with volatile this may not be safe?
I don't *think* so.
The null check is still disturbing as I think thread2 can see a "not" null ref
and the instance still not fully constructed by thread1 yet, and possibly
other complicated scenarios that I have forgot.
It shouldn't do - the write to the volatile variable should have made
sure that everything's been fully constructed before thread2 can see
it.
As there was still some
question on this and CLR memory model, I thought explicit lock (then check)
or static construction were the safe ways for now? Or has someone proved
this works in all cases?

sure that everything's been fully constructed before thread2 can see
it.

It shouldn't do - the write to the volatile variable should have made
sure that everything's been fully constructed before thread2 can see
it.

True, but I think that is any writes before the read, but does not mean all
writes have completed yet. So internal may write 1 as first step, then
thread switch happens, then finishes writing ref var (I saw this doc'd
somewhere.) Also, the ref may get written and read correctly, but fields
inside the object may not be set before a thread switch and the other thread
runs with a ref that is not fully constructed yet. I think you can force
this to happen with a bit of playing. Cheers!