Make sure fault handlers are always called when merging two chains

Author: shana

src/GitHub.Api/Tasks/TaskBase.cs

@@ -9,7 +9,7 @@ public enum TaskRunOptions
     {
         OnSuccess,
         OnFailure,
-        Always
+        OnAlways
     }
 
     public interface ITask : IAsyncResult
@@ -72,9 +72,9 @@ public abstract class TaskBase : ITask
 
         protected TaskBase continuationOnSuccess;
         protected TaskBase continuationOnFailure;
-        protected TaskBase continuationAlways;
+        protected TaskBase continuationOnAlways;
 
-        protected event Func<Exception, bool> faultHandler;
+        protected event Func<Exception, bool> catchHandler;
         private event Action finallyHandler;
         protected event Action<IProgress> progressHandler;
 
@@ -135,11 +135,29 @@ public virtual T Then<T>(T nextTask, TaskRunOptions runOptions = TaskRunOptions.
             firstTaskBase.SetDependsOn(this);
 
             if (runOptions == TaskRunOptions.OnSuccess)
+            {
                 this.continuationOnSuccess = firstTaskBase;
+                // if there are fault handlers in the chain we're appending, propagate them
+                // up this chain as well
+                if (firstTaskBase.continuationOnFailure != null)
+                    SetFaultHandler(firstTaskBase.continuationOnFailure);
+                else if (firstTaskBase.continuationOnAlways != null)
+                    SetFaultHandler(firstTaskBase.continuationOnAlways);
+                if (firstTaskBase.catchHandler != null)
+                    Catch(firstTaskBase.catchHandler);
+                if (firstTaskBase.finallyHandler != null)
+                    Finally(firstTaskBase.finallyHandler);
+            }
             else if (runOptions == TaskRunOptions.OnFailure)
+            {
                 this.continuationOnFailure = firstTaskBase;
+                DependsOn?.SetFaultHandler(firstTaskBase);
+            }
             else
-                this.continuationAlways = firstTaskBase;
+            {
+                this.continuationOnAlways = firstTaskBase;
+                DependsOn?.SetFaultHandler(firstTaskBase);
+            }
             return nextTask;
         }
 
@@ -150,7 +168,7 @@ public virtual T Then<T>(T nextTask, TaskRunOptions runOptions = TaskRunOptions.
         public ITask Catch(Action<Exception> handler)
         {
             Guard.ArgumentNotNull(handler, "handler");
-            faultHandler += e => { handler(e); return false; };
+            catchHandler += e => { handler(e); return false; };
             DependsOn?.Catch(handler);
             return this;
         }
@@ -162,7 +180,7 @@ public ITask Catch(Action<Exception> handler)
         public ITask Catch(Func<Exception, bool> handler)
         {
             Guard.ArgumentNotNull(handler, "handler");
-            faultHandler += handler;
+            catchHandler += handler;
             DependsOn?.Catch(handler);
             return this;
         }
@@ -188,10 +206,10 @@ public ITask Finally<T>(T taskToContinueWith)
             where T : ITask
         {
             Guard.ArgumentNotNull(taskToContinueWith, nameof(taskToContinueWith));
-            continuationAlways = (TaskBase)(object)taskToContinueWith;
-            continuationAlways.SetDependsOn(this);
-            DependsOn?.SetFaultHandler(continuationAlways);
-            return continuationAlways;
+            continuationOnAlways = (TaskBase)(object)taskToContinueWith;
+            continuationOnAlways.SetDependsOn(this);
+            DependsOn?.SetFaultHandler(continuationOnAlways);
+            return continuationOnAlways;
         }
 
         internal void SetFaultHandler(TaskBase handler)
@@ -275,12 +293,12 @@ protected virtual void RunContinuation()
                     TaskManager.GetScheduler(continuationOnFailure.Affinity));
             }
 
-            if (continuationAlways != null)
+            if (continuationOnAlways != null)
             {
                 //Logger.Trace($"Setting ContinueWith {Affinity} {continuation}");
-                Task.ContinueWith(_ => ((TaskBase)(object)continuationAlways).Run(), Token,
+                Task.ContinueWith(_ => ((TaskBase)(object)continuationOnAlways).Run(), Token,
                     runAlwaysOptions,
-                    TaskManager.GetScheduler(continuationAlways.Affinity));
+                    TaskManager.GetScheduler(continuationOnAlways.Affinity));
             }
         }
 
@@ -338,17 +356,17 @@ protected virtual void RaiseOnStart()
         protected virtual void RaiseOnEnd()
         {
             OnEnd?.Invoke(this);
-            if (continuationOnSuccess == null && continuationOnFailure == null && continuationAlways == null)
+            if (continuationOnSuccess == null && continuationOnFailure == null && continuationOnAlways == null)
                 finallyHandler?.Invoke();
             //Logger.Trace($"Finished {ToString()}");
         }
 
         protected virtual bool RaiseFaultHandlers(Exception ex)
         {
-            if (faultHandler == null)
+            if (catchHandler == null)
                 return false;
             bool handled = false;
-            foreach (var handler in faultHandler.GetInvocationList())
+            foreach (var handler in catchHandler.GetInvocationList())
             {
                 handled |= (bool)handler.DynamicInvoke(new object[] { ex });
                 if (handled)
@@ -459,7 +477,7 @@ public override T Then<T>(T continuation, TaskRunOptions runOptions = TaskRunOpt
         public new ITask<TResult> Catch(Action<Exception> handler)
         {
             Guard.ArgumentNotNull(handler, "handler");
-            faultHandler += e => { handler(e); return false; };
+            catchHandler += e => { handler(e); return false; };
             DependsOn?.Catch(handler);
             return this;
         }
@@ -472,7 +490,7 @@ public override T Then<T>(T continuation, TaskRunOptions runOptions = TaskRunOpt
         public new ITask<TResult> Catch(Func<Exception, bool> handler)
         {
             Guard.ArgumentNotNull(handler, "handler");
-            faultHandler += handler;
+            catchHandler += handler;
             DependsOn?.Catch(handler);
             return this;
         }
@@ -491,15 +509,15 @@ public ITask<TResult> Finally(Action<TResult> handler)
         public ITask<TResult> Finally(Func<bool, Exception, TResult, TResult> continuation, TaskAffinity affinity = TaskAffinity.Concurrent)
         {
             Guard.ArgumentNotNull(continuation, "continuation");
-            var ret = Then(new FuncTask<TResult, TResult>(Token, continuation) { Affinity = affinity, Name = "Finally" }, TaskRunOptions.Always);
+            var ret = Then(new FuncTask<TResult, TResult>(Token, continuation) { Affinity = affinity, Name = "Finally" }, TaskRunOptions.OnAlways);
             DependsOn?.SetFaultHandler(ret);
             return ret;
         }
 
         public ITask Finally(Action<bool, Exception, TResult> continuation, TaskAffinity affinity = TaskAffinity.Concurrent)
         {
             Guard.ArgumentNotNull(continuation, "continuation");
-            var ret = Then(new ActionTask<TResult>(Token, continuation) { Affinity = affinity, Name = "Finally" }, TaskRunOptions.Always);
+            var ret = Then(new ActionTask<TResult>(Token, continuation) { Affinity = affinity, Name = "Finally" }, TaskRunOptions.OnAlways);
             DependsOn?.SetFaultHandler(ret);
             return ret;
         }
@@ -542,7 +560,7 @@ protected override void RaiseOnStart()
         protected virtual void RaiseOnEnd(TResult result)
         {
             OnEnd?.Invoke(this, result);
-            if (continuationOnSuccess == null && continuationOnFailure == null && continuationAlways == null)
+            if (continuationOnSuccess == null && continuationOnFailure == null && continuationOnAlways == null)
                 finallyHandler?.Invoke(result);
             //Logger.Trace($"Finished {ToString()} {result}");
         }

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/HistoryView.cs

@@ -677,7 +677,7 @@ private void Pull()
                             // whether pull triggered a merge or a rebase, and abort the operation accordingly
                             // (either git rebase --abort or git merge --abort)
                         }
-                    }, TaskRunOptions.Always)
+                    }, TaskRunOptions.OnAlways)
                     .FinallyInUI((success, e) => {
                         if (success)
                         {

src/tests/TaskSystemIntegrationTests/Tests.cs

@@ -666,6 +666,25 @@ public async Task StartAwaitSafelyAwaits()
                 .Catch(_ => { });
             await task.StartAwait(_ => { });
         }
+
+        [Test]
+        public async Task TaskOnFailureGetsCalledWhenExceptionHappensUpTheChain()
+        {
+            var runOrder = new List<string>();
+            var exceptions = new List<Exception>();
+            var task = new ActionTask(Token, _ => { throw new InvalidOperationException(); })
+                .Then(_ => { runOrder.Add("1"); })
+                .Catch(ex => exceptions.Add(ex))
+                .Then(() => runOrder.Add("OnFailure"), TaskRunOptions.OnFailure)
+                .Finally((s, e) => { });
+            await task.StartAndSwallowException();
+            CollectionAssert.AreEqual(
+                new string[] { typeof(InvalidOperationException).Name },
+                exceptions.Select(x => x.GetType().Name).ToArray());
+            CollectionAssert.AreEqual(
+                new string[] { "OnFailure" },
+                runOrder);
+        }
     }
 
     [TestFixture]