Properly handle PropertyChanged event raisers that return Task (#346)

* Properly handle PropertyChanged event raisers that return Task * added delay option to EventTester.TestProperty to eliminate race condition when property changed raiser returns Task.
Fody · Jul 22, 2018 · e82d63b · e82d63b
1 parent 00141e5
commit e82d63b
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 18 deletions.
diff --git a/PropertyChanged.Fody/PropertyWeaver.cs b/PropertyChanged.Fody/PropertyWeaver.cs
@@ -186,27 +186,30 @@ bool ContainsCallToMethod(string onChangingMethodName)
 
     int AddSimpleInvokerCall(int index, PropertyDefinition property)
     {
-        return instructions.Insert(index,
+        index = instructions.Insert(index,
                                    Instruction.Create(OpCodes.Ldarg_0),
-                                   Instruction.Create(OpCodes.Ldstr, property.Name),
-                                   CallEventInvoker(property));
+                                   Instruction.Create(OpCodes.Ldstr, property.Name));
+
+        return instructions.Insert(index, CallEventInvoker(property).ToArray());
     }
 
     int AddPropertyChangedArgInvokerCall(int index, PropertyDefinition property)
     {
-        return instructions.Insert(index,
+        index = instructions.Insert(index,
                                    Instruction.Create(OpCodes.Ldarg_0),
-                                   Instruction.Create(OpCodes.Ldsfld, moduleWeaver.EventArgsCache.GetEventArgsField(property.Name)),
-                                   CallEventInvoker(property));
+                                   Instruction.Create(OpCodes.Ldsfld, moduleWeaver.EventArgsCache.GetEventArgsField(property.Name)));
+
+        return instructions.Insert(index, CallEventInvoker(property).ToArray());
     }
 
     int AddSenderPropertyChangedArgInvokerCall(int index, PropertyDefinition property)
     {
-        return instructions.Insert(index,
+        index = instructions.Insert(index,
                                    Instruction.Create(OpCodes.Ldarg_0),
                                    Instruction.Create(OpCodes.Ldarg_0),
-                                   Instruction.Create(OpCodes.Ldsfld, moduleWeaver.EventArgsCache.GetEventArgsField(property.Name)),
-                                   CallEventInvoker(property));
+                                   Instruction.Create(OpCodes.Ldsfld, moduleWeaver.EventArgsCache.GetEventArgsField(property.Name)));
+
+        return instructions.Insert(index, CallEventInvoker(property).ToArray());
     }
 
     int AddBeforeAfterGenericInvokerCall(int index, PropertyDefinition property)
@@ -222,9 +225,9 @@ int AddBeforeAfterGenericInvokerCall(int index, PropertyDefinition property)
             Instruction.Create(OpCodes.Ldarg_0),
             Instruction.Create(OpCodes.Ldstr, property.Name),
             Instruction.Create(OpCodes.Ldloc, beforeVariable),
-            Instruction.Create(OpCodes.Ldloc, afterVariable),
-            CallEventInvoker(property)
-            );
+            Instruction.Create(OpCodes.Ldloc, afterVariable));
+
+        index = instructions.Insert(index, CallEventInvoker(property).ToArray());
 
         return AddBeforeVariableAssignment(index, property, beforeVariable);
     }
@@ -242,9 +245,9 @@ int AddBeforeAfterInvokerCall(int index, PropertyDefinition property)
             Instruction.Create(OpCodes.Ldarg_0),
             Instruction.Create(OpCodes.Ldstr, property.Name),
             Instruction.Create(OpCodes.Ldloc, beforeVariable),
-            Instruction.Create(OpCodes.Ldloc, afterVariable),
-            CallEventInvoker(property)
-            );
+            Instruction.Create(OpCodes.Ldloc, afterVariable));
+
+        index = instructions.Insert(index, CallEventInvoker(property).ToArray());
 
         return AddBeforeVariableAssignment(index, property, beforeVariable);
     }
@@ -300,7 +303,7 @@ int InsertVariableAssignmentFromCurrentValue(int index, PropertyDefinition prope
         return index + 4;
     }
 
-    public Instruction CallEventInvoker(PropertyDefinition propertyDefinition)
+    public IEnumerable<Instruction> CallEventInvoker(PropertyDefinition propertyDefinition)
     {
         var method = typeNode.EventInvoker.MethodReference;
 
@@ -311,7 +314,14 @@ public Instruction CallEventInvoker(PropertyDefinition propertyDefinition)
             method = genericMethod;
         }
 
-        return Instruction.Create(OpCodes.Callvirt, method);
+        var instructionList =  new List<Instruction>{Instruction.Create(OpCodes.Callvirt, method)};
+
+        if(method.ReturnType.FullName != typeSystem.VoidReference.FullName)
+        {
+            instructionList.Add(Instruction.Create(OpCodes.Pop));
+        }
+
+        return instructionList;
     }
 
     public Instruction CreateIsChangedInvoker()

diff --git a/TestAssemblies/AssemblyToProcess/ClassWithTaskReturningPropertyChangedNotifier.cs b/TestAssemblies/AssemblyToProcess/ClassWithTaskReturningPropertyChangedNotifier.cs
@@ -0,0 +1,25 @@
+using System.ComponentModel;
+using System.Runtime.CompilerServices;
+using System.Threading.Tasks;
+
+public class ClassWithTaskReturningPropertyChangedNotifier : INotifyPropertyChanged
+{
+    public string Property1 { get; set; }
+    public event PropertyChangedEventHandler PropertyChanged;
+
+    public virtual Task RaisePropertyChanged([CallerMemberName] string whichProperty = "")
+    {
+        var changedArgs = new PropertyChangedEventArgs(whichProperty);
+        return RaisePropertyChanged(changedArgs);
+    }
+
+    private async Task RaisePropertyChanged(PropertyChangedEventArgs changedArgs)
+    {
+        void raiseChange()
+        {
+            PropertyChanged?.Invoke(this, changedArgs);
+        }
+
+        await Task.Run(() => raiseChange());
+    }
+}
diff --git a/Tests/AssemblyToProcessTests.cs b/Tests/AssemblyToProcessTests.cs
@@ -73,6 +73,13 @@ public void ClassWithIndirectImplementation()
         EventTester.TestProperty(instance, false);
     }
 
+    [Fact]
+    public void ClassWithTaskReturningPropertyChangedNotifier()
+    {
+        var instance = testResult.GetInstance("ClassWithTaskReturningPropertyChangedNotifier");
+        EventTester.TestProperty(instance, false, true);
+    }
+
     [Fact]
     public void UseSingleEventInstance()
     {

diff --git a/Tests/EventTester.cs b/Tests/EventTester.cs
@@ -2,6 +2,7 @@
 using System.ComponentModel;
 using System.Linq;
 using System.Reflection;
+using System.Threading.Tasks;
 using Xunit;
 
 public static class EventTester
@@ -20,7 +21,7 @@ internal static void TestPropertyNotCalled(dynamic instance)
         Assert.False(property1EventCalled);
     }
 
-    internal static void TestProperty(dynamic instance, bool checkProperty2)
+    internal static void TestProperty(dynamic instance, bool checkProperty2, bool delayAfterSetProperty1 = false)
     {
         var property1EventCalled = false;
         var property2EventCalled = false;
@@ -38,6 +39,11 @@ internal static void TestProperty(dynamic instance, bool checkProperty2)
         };
         instance.Property1 = "a";
 
+        if(delayAfterSetProperty1)
+        {
+            Task.Delay(TimeSpan.FromMilliseconds(25)).Wait();
+        }
+
         Assert.True(property1EventCalled);
         if (checkProperty2)
         {