Merge pull request #7 from wlindley/command-relay-concurrency-fix

Fix for bug caused by concurrent command chain execution

Bantam/Bantam.csproj

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <PropertyGroup>
     <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
@@ -31,6 +31,7 @@
     <Reference Include="System" />
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="MultiLock.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />
     <Compile Include="Poolable.cs" />
     <Compile Include="ObjectPool.cs" />

Bantam/CommandChainExecutor.cs

@@ -73,6 +73,7 @@ internal void Start(Event triggeringEvent, CommandChain chain, CommandRelay mana
 			this.pool = pool;
 			failureAllocator = chain.FailureCommand;
 			enumerator = chain.Commands.GetEnumerator();
+			pool.Lock(triggeringEvent, this);
 			enumerator.MoveNext();
 			Next();
 		}
@@ -84,7 +85,10 @@ public void CurrentCommandComplete()
 			if (enumerator.MoveNext())
 				Next();
 			else
+			{
+				pool.Unlock(triggeringEvent.GetType(), triggeringEvent, this);
 				manager.CompleteChainExecution<EventCommandChainExecutor>(this);
+			}
 		}
 
 		public void CurrentCommandFailed()

Bantam/MultiLock.cs

@@ -0,0 +1,25 @@
+using System.Collections.Generic;
+
+namespace Bantam
+{
+	public class MultiLock : Poolable
+	{
+		public bool IsLocked { get { return keys.Count > 0; } }
+		private readonly Dictionary<object, bool> keys = new Dictionary<object, bool>(); 
+
+		public void Lock(object key)
+		{
+			keys[key] = true;
+		}
+
+		public void Unlock(object key)
+		{
+			keys.Remove(key);
+		}
+
+		public void Reset()
+		{
+			keys.Clear();
+		}
+	}
+}

Bantam/ObjectPool.cs

@@ -1,44 +1,78 @@
 using System;
-using System.Collections;
 using System.Collections.Generic;
 
 namespace Bantam
 {
 	public class ObjectPool
 	{
-		private Dictionary<Type, Queue<Poolable>> instances = new Dictionary<Type, Queue<Poolable>>();
 		public Dictionary<Type, int> UniqueInstances = new Dictionary<Type, int>();
 
+		private readonly Dictionary<Type, Queue<Poolable>> instances = new Dictionary<Type, Queue<Poolable>>();
+		private readonly Dictionary<Poolable, MultiLock> lockedInstances = new Dictionary<Poolable, MultiLock>();
+
 		public T Allocate<T>() where T : class, Poolable, new()
 		{
 			EnsurePoolExists<T>();
-			return GetInstance<T>();
+			var instance = GetInstance<T>();
+			Lock(instance, this);
+			return instance;
 		}
 
 		public Poolable Allocate(Type type)
 		{
 			ValidateType(type);
 			EnsurePoolExists(type);
-			return GetInstance(type);
+			var instance = GetInstance(type);
+			Lock(instance, this);
+			return instance;
 		}
 
 		public void Free<T>(T instance) where T : Poolable
 		{
 			if (null == instance)
 				throw new NullInstanceException();
-			EnsurePoolExists<T>();
-			instances[typeof(T)].Enqueue(instance);
+			Unlock(instance, this);
 		}
 
 		public void Free(Type type, Poolable instance)
+		{
+			Unlock(type, instance, this);
+		}
+
+		public void Lock(Poolable instance, object key)
+		{
+			EnsureLockExists(instance);
+			lockedInstances[instance].Lock(key);
+		}
+
+		public void Unlock<T>(T instance, object key) where T : Poolable
+		{
+			Unlock(typeof(T), instance, key);
+		}
+
+		public void Unlock(Type type, Poolable instance, object key)
+		{
+			Validate(type, instance);
+			MultiLock instanceLock;
+			lockedInstances.TryGetValue(instance, out instanceLock);
+			if (null == instanceLock)
+				return;
+			instanceLock.Unlock(key);
+			if (!instanceLock.IsLocked)
+			{
+				lockedInstances.Remove(instance);
+				FreeInternalLock(instanceLock);
+				InternalFree(type, instance);
+			}
+		}
+
+		private void Validate(Type type, Poolable instance)
 		{
 			if (null == instance)
 				throw new NullInstanceException();
 			ValidateType(type);
 			if (!type.IsInstanceOfType(instance))
 				throw new MismatchedTypeException();
-			EnsurePoolExists(type);
-			instances[type].Enqueue(instance);
 		}
 
 		private void EnsurePoolExists<T>()
@@ -55,6 +89,12 @@ private void EnsurePoolExists(Type type)
 			}
 		}
 
+		private void EnsureLockExists(Poolable instance)
+		{
+			if (!lockedInstances.ContainsKey(instance))
+				lockedInstances[instance] = AllocateInternalLock();
+		}
+
 		private void ValidateType(Type type)
 		{
 			if (!type.IsClass)
@@ -93,6 +133,24 @@ private Poolable GetInstance(Type type)
 			instance.Reset();
 			return instance;
 		}
+
+		private MultiLock AllocateInternalLock()
+		{
+			EnsurePoolExists<MultiLock>();
+			return GetInstance<MultiLock>();
+		}
+
+		private void FreeInternalLock(MultiLock internalLock)
+		{
+			EnsurePoolExists<MultiLock>();
+			instances[typeof(MultiLock)].Enqueue(internalLock);
+		}
+
+		private void InternalFree(Type type, Poolable instance)
+		{
+			EnsurePoolExists(type);
+			instances[type].Enqueue(instance);
+		}
 	}
 
 	public class ObjectPoolException : Exception {}

BantamTest/BantamTest.csproj

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <PropertyGroup>
     <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
@@ -28,26 +28,31 @@
     <ConsolePause>false</ConsolePause>
   </PropertyGroup>
   <ItemGroup>
-    <Reference Include="System" />
-    <Reference Include="nunit.framework">
-      <HintPath>..\packages\NUnit.2.6.4\lib\nunit.framework.dll</HintPath>
+    <Reference Include="nunit.framework, Version=3.6.1.0, Culture=neutral, PublicKeyToken=2638cd05610744eb, processorArchitecture=MSIL">
+      <HintPath>..\packages\NUnit.3.6.1\lib\net45\nunit.framework.dll</HintPath>
+      <Private>True</Private>
     </Reference>
+    <Reference Include="System" />
   </ItemGroup>
   <Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
-  <ItemGroup>
-    <None Include="packages.config" />
-  </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\Bantam\Bantam.csproj">
       <Project>{DD312BD0-4A72-4F52-8969-05313D2AEF35}</Project>
       <Name>Bantam</Name>
     </ProjectReference>
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="MultiLockTest.cs" />
     <Compile Include="ObjectPoolTest.cs" />
     <Compile Include="EventBusTest.cs" />
     <Compile Include="CommandRelayTest.cs" />
     <Compile Include="IntegrationTest.cs" />
     <Compile Include="ModelRegistryTest.cs" />
   </ItemGroup>
+  <ItemGroup>
+    <None Include="packages.config" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
 </Project>

BantamTest/CommandRelayTest.cs

@@ -1,4 +1,5 @@
-using NUnit.Framework;
+using System.Collections.Generic;
+using NUnit.Framework;
 
 namespace Bantam.Test
 {
@@ -16,6 +17,8 @@ public void SetUp()
 			testObj = new CommandRelay(eventBus, pool);
 			DummyCommand.ExecuteCount = 0;
 			DummyCommand.LastValue = 0;
+			AsyncCommand.triggeringEvents.Clear();
+			AsyncCommand.ClearAll();
 		}
 
 		[Test]
@@ -103,6 +106,26 @@ public void OnFailureCanTakeAnOptionalInitializerForCommand()
 			eventBus.Dispatch<DummyEvent>(evt => evt.value = expectedValue);
 			Assert.AreEqual(expectedValue, DummyCommand.LastValue);
 		}
+
+		[Test]
+		public void AsyncCommandChainsLockTriggeringEvent()
+		{
+			testObj.On<DummyEvent>().Do<AsyncCommand>((cmd, evt) => cmd.trigger = evt);
+			eventBus.Dispatch<DummyEvent>();
+			eventBus.Dispatch<DummyEvent>();
+			Assert.AreNotSame(AsyncCommand.triggeringEvents[0], AsyncCommand.triggeringEvents[1]);
+		}
+
+		[Test]
+		public void AsyncCommandChainsReleaseLockOnTriggeringEventWhenTheyComplete()
+		{
+			testObj.On<DummyEvent>().Do<AsyncCommand>((cmd, evt) => cmd.trigger = evt);
+			eventBus.Dispatch<DummyEvent>();
+			eventBus.Dispatch<DummyEvent>();
+			AsyncCommand.CompleteAll();
+			eventBus.Dispatch<DummyEvent>();
+			Assert.AreSame(AsyncCommand.triggeringEvents[0], AsyncCommand.triggeringEvents[2]);
+		}
 	}
 
 	public class DummyCommand : Command
@@ -126,4 +149,36 @@ public override void Execute()
 			Fail();
 		}
 	}
+
+	public class AsyncCommand : Command
+	{
+		private static List<AsyncCommand> commands = new List<AsyncCommand>();
+		public static List<Event> triggeringEvents = new List<Event>();
+		public Event trigger;
+
+		public override void Execute()
+		{
+			commands.Add(this);
+			Retain();
+			triggeringEvents.Add(trigger);
+		}
+
+		public static void CompleteAll()
+		{
+			foreach (var c in commands)
+				c.Done();
+			ClearAll();
+		}
+
+		public static void ClearAll()
+		{
+			commands.Clear();
+		}
+
+		public override void Reset()
+		{
+			base.Reset();
+			trigger = null;
+		}
+	}
 }

BantamTest/IntegrationTest.cs

@@ -62,7 +62,7 @@ public void FullIntegrationTest()
 
 			Assert.AreEqual(1, pool.UniqueInstances[typeof(LoginEvent)]);
 			Assert.AreEqual(1, pool.UniqueInstances[typeof(LoginFailedEvent)]);
-			Assert.AreEqual(1, pool.UniqueInstances[typeof(LoginSuccessEvent)]);
+			Assert.AreEqual(2, pool.UniqueInstances[typeof(LoginSuccessEvent)]);
 			Assert.AreEqual(2, pool.UniqueInstances[typeof(LoginCommand)]); //A second LoginCommand is needed to respond to the first one failing before the first LoginCommand's Done method is called.
 			Assert.AreEqual(1, pool.UniqueInstances[typeof(RecordLoginCommand)]);
 			Assert.AreEqual(1, pool.UniqueInstances[typeof(UpdateDisplayNameCommand)]);