Request for review: Possible threading issue / synchronization bug in DynamicProxy

[stakx]

@BrunoJuchli and I have once again been working on #193. In #193 (comment) he observed a NullReferenceException being thrown from some Dictionary<,> used by DynamicProxy, which got me curious.

Perhaps I'm a little sleepy and not paying full attention, but I think I have found a major threading issue in DynamicProxy. Could someone please validate the following observation:

I'm looking at the method BaseProxyGenerator.ObtainProxyType. This is the method in DynamicProxy where a proxy type is either fetched from a cache, or where its generation is triggered if it isn't in the cache yet:

Core/src/Castle.Core/DynamicProxy/Generators/BaseProxyGenerator.cs

Line 385 in 6959806

protected Type ObtainProxyType(CacheKey cacheKey, Func<string, INamingScope, Type> factory)

The following section is interesting. Take note of what happens after we get hold of an upgradeable read lock, but before we upgrade it to a write lock:

Core/src/Castle.Core/DynamicProxy/Generators/BaseProxyGenerator.cs

Lines 398 to 423 in 6959806

	// This is to avoid generating duplicate types under heavy multithreaded load.
	using (var locker = Scope.Lock.ForReadingUpgradeable())
	{
	// Only one thread at a time may enter an upgradable read lock.
	// See if an earlier lock holder populated the cache.
	cacheType = GetFromCache(cacheKey);
	if (cacheType != null)
	{
	Logger.DebugFormat("Found cached proxy type {0} for target type {1}.", cacheType.FullName, targetType.FullName);
	return cacheType;
	}
	// Log details about the cache miss
	Logger.DebugFormat("No cached proxy type was found for target type {0}.", targetType.FullName);
	EnsureOptionsOverrideEqualsAndGetHashCode(ProxyGenerationOptions);
	var name = Scope.NamingScope.GetUniqueName("Castle.Proxies." + targetType.Name + "Proxy");
	var proxyType = factory.Invoke(name, Scope.NamingScope.SafeSubScope());
	// Upgrade the lock to a write lock.
	using (locker.Upgrade())
	{
	AddToCache(cacheKey, proxyType);
	}
	return proxyType;
	}

In line 414, we are calling into Scope.NamingScope.GetUniqueName while we still don't have a write lock. However, that method writes to a private dictionary:

Core/src/Castle.Core/DynamicProxy/Generators/NamingScope.cs

Lines 39 to 54 in 6959806

	public string GetUniqueName(string suggestedName)
	{
	Debug.Assert(string.IsNullOrEmpty(suggestedName) == false,
	"string.IsNullOrEmpty(suggestedName) == false");
	int counter;
	if (!names.TryGetValue(suggestedName, out counter))
	{
	names.Add(suggestedName, 0);
	return suggestedName;
	}
	counter++;
	names[suggestedName] = counter;
	return suggestedName + "_" + counter.ToString();
	}

If my understanding of the reader-writer locks is correct, there's the possibility of any number of observers reading from that dictionary concurrently (because we're not in a write lock yet).

But what's even worse, this is (line 415) where System.Reflection.Emit happens!

I see nothing that prevents System.Reflection.Emit generation of two proxy types at the same time... and although System.Reflection.Emit does apparently some locking of its own, it doesn't actually give any official thread-safety guarantees. (Update: The upgradeable read lock should ensure that only one thread can ever trigger System.Reflection.Emit at a time. But the problematic access to NamingScope likely remains.)

Finally, we're reaching the section where we're getting hold of a write lock. This is what will block multiple readers. But the only thing it protects against multiple readers is the cache update, not the dynamic type generation itself.

This is what I believe should be happening:

-	var name = Scope.NamingScope.GetUniqueName("Castle.Proxies." + targetType.Name + "Proxy"); 
-	var proxyType = factory.Invoke(name, Scope.NamingScope.SafeSubScope()); 
  
 	// Upgrade the lock to a write lock.  
 	using (locker.Upgrade()) 
 	{ 
+		var name = Scope.NamingScope.GetUniqueName("Castle.Proxies." + targetType.Name + "Proxy"); 
+		var proxyType = factory.Invoke(name, Scope.NamingScope.SafeSubScope()); 
 		AddToCache(cacheKey, proxyType); 
 	}

Am I misunderstanding something here, or do we really have concurrent System.Reflection.Emit-ting into the same ModuleBuilder going on here?

/cc @TimLovellSmith

[TimLovellSmith]

@stakx
Heh, I had looked at this a bit and I wasn't sure. My impression was that maybe it was OK because all the actual Emitting was done on a single 'Scope', and would be within the lock so everything would be OK.

However, then I read about the NullReferenceExceptions from a dictionary in the other thread and started feeling very suspicious that there's a thread bug somewhere in Castle... It would have to be specifically in ObtainProxyType wouldn't it. At this point I had a moment of terror. I just touched that code... did I regress it? 😱

However, it looks like no, the only thing I did was make the bug more apparent 👓 , by making the write-locking scope look more like a write locking scope. 😌

So, YES, I am almost convinced you found the bug. However... there is still just one doubt on my mind. I read somewhere, I think in the comments around the locking class, that it was only possible for one thread to hold the upgradeable read lock at a time...

Oh, actually it was this very same code! Maybe the comment is just... wrong? 😨 What does MSDN say..

ReaderWriterLockSlim allows multiple threads to be in read mode, allows one thread to be in write mode with exclusive ownership of the lock, and allows one thread that has read access to be in upgradeable read mode, from which the thread can upgrade to write mode without having to relinquish its read access to the resource.

Does there can be one thread with upgradeable read access, but not yet write access, and one thread with full write access?

Apparently not:

No more than one thread at a time can enter the lock with the option to upgrade; additional threads that try to enter upgradeable mode are blocked.

and

Write: In this state, one thread has entered the lock for write access to the protected resource. That thread has exclusive possession of the lock. Any other thread that tries to enter the lock for any reason is blocked.

That sounds a lot to me like it is correct that only one thread can be in the upgradeable-read-lock-or-write-holding state at once. So I am still not sure if you found the bug? Oh... Wait. Duh. I am missing the point!

The problem is not that there are multiple writers... The problem is that since we aren't taking the write lock before writing we're not ensuring exclusive access between reads and writes. Yes. That is totally a problem! You got it! 🎉

@stakx

More simply put you found a write in a "read transaction", many can read, only one can write. For a man who claims to not know threading, I am mega impressed.

[stakx]

@Fir3pho3nixx - For me, thinking about multi-threading is about as easy as trying to juggle. You should see it someday. It's a disaster. 😆

[TimLovellSmith]

Moral of the story for me is that read-write locking is much harder than it looks! :)

Moral of the story for me is that read-write locking is much harder than it looks! :)

Read write all the things! :)

Closed via #383

[stakx]

Hmm... I'm starting to second-guess whether my PR was necessary. Perhaps taking an upgradeable read lock (which only one thread may enter at a time) was enough to protect ModuleScope.NamingScope from concurrent read/write access... because there likely are no other readers around in the first place: BaseProxyGenerator.ObtainProxyType appears to be the only place where ModuleScope.NamingScope is accessed. Likewise, if BaseProxyGenerator.ObtainProxyType is the only method where Reflection.Emit gets triggered, then there would be no need to do it inside a write lock; the upgradeable read lock would already do the job of only letting someone in at a time.

Or, in other words: Taking an exclusive write lock won't make a difference if (1) the accessed facilities are accessed nowhere else (which I think is actually the case); or (2) they are accessed in other places, but without the use of any locks at all (which would be a bug that needs to be addressed).

[TimLovellSmith]

@stakx I'm pretty sure its necessary - its bad enough that they are accessed here, but without doing the read/write locking correctly.

E.g. for the dictionary, if you have two threads, one writing (while not holding the write lock) and one reading, the reading one can see an invalid intermediate state where the dictionary data structure is only partially updated, instead of completely updated.

[stakx]

if you have two threads, one writing (while not holding the write lock) and one reading [...]

But that's the thing: Iff the method we've just changed is the only place where reads happen, and the reads happen while an upgradeable read lock is taken, then there can only by one reader at a time... so the fact that it also writes probably doesn't matter in practice!

(That said, I guess it's just more honest to take a write lock; makes it more explicit what kind of access we're performing, if nothing else.)

[TimLovellSmith]

@Stackx Edit... now I am confused as you.

[TimLovellSmith]

@stakx
Thinking about it more, I'm not sure if what you're saying is true:

Iff the method we've just changed is the only place where reads happen, and the reads happen while an upgradeable read lock is taken, then there can only by one reader at a time... so the fact that it also writes probably doesn't matter in practice!

I am going out on a limb here, but one reason for which I wonder it might not be true is that the shareable read lock might be implemented specifically with things like 'read barriers' (since you got a read lock you must be only reading memory, right?) as opposed to full mutex.

Whereas this guess that having a single thread holding the 'upgradeable read' being good enough to be ensure write safety seems to be based on a mental model where the upgradeable read lock is imagined to be a full mutex.

But I think that it is not a full mutex is kind of the point of having an upgradeable read-write lock in the first place - as opposed to just a lock that permits two modes: shareable reads versus full exclusive access (like the old Windows read write locking APIS which ReaderWriterLockSlim doesn't use).

[TimLovellSmith]

But I think that it is not a full mutex is kind of the point of having an upgradeable read-write lock in the first place - as opposed to just a lock that permits two modes: shareable reads versus full exclusive access (like the old Windows read write locking APIS which ReaderWriterLockSlim doesn't use).

Well I'm pretty sure I'm all wrong actually. The point is to let the thread that is about to take the write lock do a little bit of useful work before it actually takes the write lock and blocks all other threads, with some guarantee it won't be interrupted by someone corrupting states...

Iff the method we've just changed is the only place where reads happen, and the reads happen while an upgradeable read lock is taken, then there can only by one reader at a time..

You can have multiple readers outside the context of a write. You have locked readers when a write happens. Even if it is unnecessary(based on usage), it is still a valuable change.