[Mono-devel-list] Patch for Thread.GetNamedDataSlot

[Sébastien Robitaille]

Hi Jonathan,

thanks for the review.
For your information, I used lock(typeof(Thread)) in GetNamedDataSlot because there is the same kind of locking in InitDataStoreHash, AllocateNamedDataSlot and FreeNamedDataSlot. I agree with you that "lock(typeof(Thread)) should not be used .. That means that these methods must also be modified...


Before I submit an updated patch with these methods, I need advice on this:
I can use a different object for the lock as suggested. But since there is already a static Hashtable object available (which all these methods are using), I could use it instead (to avoid allocating a new object). But in this scenario, the Hashtable object would have to be allocated directly at the declaration instead of using the InitDataStoreHash() method "on demand"...

i.e.
private static Hashtable datastorehash = new Hashtable();

I think this scenario is more clean, but I don't know the cost of always allocating a Hashtable object vs always allocating a System.Object.

Another point: In these methods, the HashTable object is created Synchronized. I think that it is superfluous since every access to that hashtable is locked anyway...

All comments/suggestions are welcome.

Regards
Sébas


-----Original Message-----
From: mono-devel-list-admin at lists.ximian.com
[mailto:mono-devel-list-admin at lists.ximian.com]On Behalf Of Jonathan
Pryor
Sent: Thursday, July 15, 2004 7:08 AM
To: Sébastien Robitaille
Cc: Mono Development List; dick at ximian.com
Subject: Re: [Mono-devel-list] Patch for Thread.GetNamedDataSlot


On Wed, 2004-07-14 at 21:14, Sébastien Robitaille wrote:
> The problem: Thread.GetNamedDataSlot is not thread-safe. There is an
> exception thrown if multiple threads are calling this method at
> the same time.

<snip/>

+                       lock(typeof(Thread))
+                       {
+                               if (datastorehash == null)

<snip/>

You shouldn't lock the Thread class.  It is error prone and can lead to
deadlock.  You should instead lock a private object:

	private static object _lock = new object ();

	// ...
		lock (_lock) {
			// ...
		}

See also:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnaskdr/html/askgui06032003.asp

 - Jon


_______________________________________________
Mono-devel-list mailing list
Mono-devel-list at lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list