MBR IT/.NET 247 : Thread safety ? on microsoft.public.dotnet.general

Thread safety ?

Messages

Related Types

This message was discovered on microsoft.public.dotnet.general.
Responses highlighted in red are from those people who are likely to be able to contribute good, authoratitive information to this discussion. They include Microsoft employees, MVP's and others who IMHO contribute well to these kinds of discussions.

Post a new message to this list...

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.

Reply to this message...

Patrik Löwendahl [C# MVP] (VIP)

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.

--
Patrik Löwendahl [C# MVP]
www.cshrp.net - "Elegant code by witty programmers"

"Support" <Click here to reveal e-mail address> wrote in message
news:etq4V$Click here to reveal e-mail address...
[Original message clipped]

Reply to this message...

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…

Reply to this message...

Jon Skeet [C# MVP] (VIP)

Phil Jenson <Click here to reveal e-mail address> wrote:
[Original message clipped]

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 - <Click here to reveal e-mail address>
http://www.pobox.com/~skeet
If replying to the group, please do not mail me too

Reply to this message...

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..

Reply to this message...

William Stacey [MVP] (VIP)

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?

--
William Stacey, MVP

"Jon Skeet [C# MVP]" <Click here to reveal e-mail address> wrote in message
news:Click here to reveal e-mail address...
[Original message clipped]

Reply to this message...

Jon Skeet [C# MVP] (VIP)

William Stacey [MVP] <Click here to reveal e-mail address> wrote:
> Didn't we conclude before that even with volatile this may not be safe?

I don't *think* so.

[Original message clipped]

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.

[Original message clipped]

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

--
Jon Skeet - <Click here to reveal e-mail address>
http://www.pobox.com/~skeet
If replying to the group, please do not mail me too

Reply to this message...

William Stacey [MVP] (VIP)

[Original message clipped]

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!

--
William Stacey, MVP

Reply to this message...

Jon Skeet [C# MVP] (VIP)

William Stacey [MVP] <Click here to reveal e-mail address> wrote:
[Original message clipped]

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>

--
Jon Skeet - <Click here to reveal e-mail address>
http://www.pobox.com/~skeet
If replying to the group, please do not mail me too

Reply to this message...

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,

--
Ignacio Machin,
ignacio.machin AT dot.state.fl.us
Florida Department Of Transportation

"Support" <Click here to reveal e-mail address> wrote in message
news:etq4V$Click here to reveal e-mail address...
[Original message clipped]

Reply to this message...

Jon Skeet [C# MVP] (VIP)

Support <Click here to reveal e-mail address> wrote:
[Original message clipped]

Any reason to use this complicated pattern rather than a simple one as
specified on

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

?

[Original message clipped]

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.

--
Jon Skeet - <Click here to reveal e-mail address>
http://www.pobox.com/~skeet
If replying to the group, please do not mail me too

Reply to this message...

	System.Object
	System.Security.SecurityManager
	System.Web.HttpContext

MBR BootFX
Best-of-breed application framework for .NET projects, developed by Matthew Baxter-Reynolds and MBR IT

	Copyright © Matthew Baxter-Reynolds 2001-2008. '.NET 247 Software Development Services' is a trading style of MBR IT Solutions Ltd.
	Contact Us - Terms of Use - Privacy Policy - www.dotnet247.com