Thread safety ??

[Support]

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.

 

[Jon Skeet [C# MVP]]

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 ?

Well, that depends on what IsAllowed *actually* does. Two threads will
certainly be able to call it at the same time, but for many things
that's just fine. If, on the other hand, IsAllowed needs to read and
write some variables from the singleton, it may *not* be threadsafe.

Put it this way: being part of a singleton isn't relevant here. If it
would normally be okay for two threads to execute your method at a
time, that's fine - otherwise you'll need locking just as you would
elsewhere.

 

[Ignacio Machin \( .NET/ C# MVP \)]

Hi,

You should take a look at Jon Skeet article about singleton in
http://www.yoda.arachsys.com/csharp/singleton.html It explain in details how
to make it thread safe in the more efficient way.

Regarding your IsAllowed method, as long as you don't use any instance
variable you are fine, if you use an instance variable you should take care
of sync. the access to it.

Cheers,

 

[Patrik Löwendahl [C# MVP]]

I can't see anything in that pseudo-code worth looking.

If the method isn't accessing shared resources, there's no reason for
locking. The HttpContext.Current will be unique for each user who sent a
HTTP request to your application and aren't shared between users.

Also, for just reading there's seldom any need for locking, if the object
isn't written to from some other thread in your application.

 

[Phil Jenson]

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.

Phil…

 

[Jon Skeet [C# MVP]]

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.

 

[Phil Jenson]

Jon

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

Thanks for the feedback. Will look into this further.

Phil..

 

[William Stacey [MVP]]

Didn't we conclude before that even with volatile this may not be safe? 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. 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?

 

[Jon Skeet [C# MVP]]

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?

I think the discussion from a while ago was trying to find a way of
avoiding it being volatile.

 

[William Stacey [MVP]]

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!

 

[Jon Skeet [C# MVP]]

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!

No, the point of the volatile write is that all "pending" writes happen
before it.

From the spec:

<quote>
A volatile write has "release semantics" meaning that the write is
guaranteed to happen after any memory references prior to the write
instruction in the CIL instruction sequence.
</quote>