Safety of static class/variable/method

  • Thread starter Thread starter Patrick
  • Start date Start date
P

Patrick

Is the following use of static variable and method "safe"? It is in a Class
Library that is invoked from an ASP.NET User Control of many ASP.NET pages?
It seems to be working, but I recall having some obscure issues with the
initialisation of "shared" variables in VB.NET with old .NET 1.0 (the
following is .NET 1.1 on Win2K SP4).

public class Constants
{
//The static variable in question
private static Hashtable colLookupValues;

public Constants()
{
}

//The static method in question
public static ArrayList getHashValue(string strKey)
{
ArrayList colHashValues = new ArrayList();
string strCurHashKey = strKey;

//Is this OK?
if (colLookupValues==null)
{
colLookupValues = new Hashtable();
colLookupValues.Add(KEY1,OBJ1);
colLookupValues.Add(OBJ1,OBJ2);
colLookupValues.Add(OBJ2,OBJ3);
}

//Do Lookup and return a list

} //end function getHashValue
}
}
 
Patrick said:
Is the following use of static variable and method "safe"? It is in a Class
Library that is invoked from an ASP.NET User Control of many ASP.NET pages?
It seems to be working, but I recall having some obscure issues with the
initialisation of "shared" variables in VB.NET with old .NET 1.0 (the
following is .NET 1.1 on Win2K SP4).
Click to expand...

It's not threadsafe in that several different threads could all get to
the test for nullity and then populate a new hashtable. In general, all
access to shared data should be synchronized.

I'm writing an article about multi-threading which will cover this, but
I'm afraid it's a way off being finished atm.
 
If I do make access to the method synchronised, the ASP.NET threads could be
blocked waiting to call the method!

Would I not be better off leaving things as is, and making colLookupValues
local variable within the method. e.g.

public class Constants
{

public Constants()
{
}

//The static method in question
public static ArrayList getHashValue(string strKey)
{

//The static variable in question
Hashtable colLookupValues;

ArrayList colHashValues = new ArrayList();
string strCurHashKey = strKey;

//Is this OK?
if (colLookupValues==null)
{
colLookupValues = new Hashtable();
colLookupValues.Add(KEY1,OBJ1);
colLookupValues.Add(OBJ1,OBJ2);
colLookupValues.Add(OBJ2,OBJ3);
}

//Do Lookup and return a list

} //end function getHashValue
}
}
 
Or how about this:

public class Constants
{
//The static variable in question
private static Hashtable colLookupValues;

public Constants()
{
}

//The static method in question
public static ArrayList getHashValue(string strKey)
{
ArrayList colHashValues = new ArrayList();
string strCurHashKey = strKey;

//Is this OK?
if (colLookupValues==null)
{
lock(this)
{
colLookupValues = new Hashtable();
colLookupValues.Add(KEY1,OBJ1);
colLookupValues.Add(OBJ1,OBJ2);
colLookupValues.Add(OBJ2,OBJ3);
}
}

//Do Lookup and return a list

} //end function getHashValue
}
}
 
Patrick said:
If I do make access to the method synchronised, the ASP.NET threads could be
blocked waiting to call the method!
Click to expand...

It's not going to take long though, is it? They're unlikely to get
blocked for any significant amount of time. You don't need to make the
whole method synchronized - just put a lock around the whole of the

if (colLookupValues==null) section. Note that the lock needs to go
*round* that section rather than within it. Double-checked locking
doesn't work in .NET. (Or rather, it's not guaranteed to.)
Would I not be better off leaving things as is, and making colLookupValues
local variable within the method. e.g.
Click to expand...

Well, you're then creating a new hashtable for every call, which I'd
expect to be more expensive than taking out a very brief lock.
 
Patrick said:
Or how about this:
Click to expand...

<snip>

No - that's double-checked locking, which doesn't work in .NET. Put the
lock *around* the test and it's fine though.

Personally I wouldn't lock on "this" though, as other classes may
decide to lock on the instance. Create a variable solely for locking
purposes. In fact in this case you can't lock on "this" as it's a
static method, but as a general rule I'd suggest locking on private
variables. For instance in this case:

static object colLookupValuesLock = new object();
 
Hey Patrick

Instead you could use a static constructor. Statis constructors are thread safe by definition and are guaranteed to run before any other method on your type is executed

public class Constant

//The static variable in questio
private static Hashtable colLookupValues

static Constants(

colLookupValues = new Hashtable()
colLookupValues.Add(KEY1,OBJ1)
colLookupValues.Add(OBJ1,OBJ2)
colLookupValues.Add(OBJ2,OBJ3)


public Constants(



//The static method in questio
public static ArrayList getHashValue(string strKey

ArrayList colHashValues = new ArrayList()
string strCurHashKey = strKey

//Do Lookup and return a lis

} //end function getHashValu



Regards, Jakob
 
I suspect it make sense to do the locking like
lock(this)
{
if (colLookupValues==null)
{
colLookupValues = new Hashtable();
colLookupValues.Add(KEY1,OBJ1);
colLookupValues.Add(OBJ1,OBJ2);
colLookupValues.Add(OBJ2,OBJ3);
}
}

However, I have just had a quick read of
http://msdn.microsoft.com/library/d...enref/html/cpconthreadingdesignguidelines.asp

I suspect the example to use System.Threading.Interlocked.CompareExchange
(which is apparently more efficient that what I am doing above) is not going
to work, because I am initialise a hashtable that needs to be initialised?
 
You need to acquire the lock before you test for colLookupValues == null. That is, the if-statement has to be nested within the scope of the lock, because another thread may initialize the colLookupValues member between the test for colLookupValues an your lock statement

Regards, Jakob.
 
Patrick said:
I suspect it make sense to do the locking like
lock(this)
{
if (colLookupValues==null)
{
colLookupValues = new Hashtable();
colLookupValues.Add(KEY1,OBJ1);
colLookupValues.Add(OBJ1,OBJ2);
colLookupValues.Add(OBJ2,OBJ3);
}
}

However, I have just had a quick read of
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpge
nref/html/cpconthreadingdesignguidelines.asp

I suspect the example to use System.Threading.Interlocked.CompareExchange
(which is apparently more efficient that what I am doing above) is not going
to work, because I am initialise a hashtable that needs to be initialised?
Click to expand...

I don't know enough about CompareExchange to say for sure whether or
not it could help - but the code above is definitely a safe way of
working, and is the approach I'd recommend.
 
Patrick said:
I suspect it make sense to do the locking like
lock(this)
{
if (colLookupValues==null)
{
colLookupValues = new Hashtable();
colLookupValues.Add(KEY1,OBJ1);
colLookupValues.Add(OBJ1,OBJ2);
colLookupValues.Add(OBJ2,OBJ3);
}
}

However, I have just had a quick read of
http://msdn.microsoft.com/library/d...enref/html/cpconthreadingdesignguidelines.asp

I suspect the example to use System.Threading.Interlocked.CompareExchange
(which is apparently more efficient that what I am doing above) is not
going
to work, because I am initialise a hashtable that needs to be initialised?
Click to expand...
In this case, CompareExchange could work, using code like below, if your
code is written for it. There are two issues you have to deal with though.
Hashtable x = new Hashtable();
x.Add(KEY1,OBJ1);
x.Add(OBJ1,OBJ2);
x.Add(OBJ2,OBJ3);
Interlocked.CompareExchange(ref colLookupValues,x, null);

First, this will only work if all threads can use colLookupValues
transparently(ie, without knowing or caring about the particular instance
and being able to deal with any state changes involved). This means no local
variables containing a reference to the hashtable, you couldn't add data to
the table and you would have to write your access code so it could deal with
a key disappearing at any given time(between calls to ContainsKey and the
indexer or even subsequent indexer calls). This will likely be harder to
write than it sounds.
The other problem with it is you have to create and fill the hash table even
if colLookValues is already there, which isn't particularly efficent.

I would go with locking. I doubt using CompareExchange is going to be
efficent enough to make up for the more complex code you'll have to write.
 
Back
Top