Fix running TaskQueue and catching errors

shana · shana · commit 9cdc8742dcf8 · 2018-05-31T17:30:29.000+02:00
diff --git a/src/GitHub.Api/Git/RepositoryManager.cs b/src/GitHub.Api/Git/RepositoryManager.cs
@@ -460,7 +460,7 @@ private ITask HookupHandlers(ITask task, bool filesystemChangesExpected)
                 }
             };
 
-            task.Finally(success =>
+            task.OnEnd += (_, __, ___) =>
             {
                 if (filesystemChangesExpected)
                 {
@@ -473,6 +473,21 @@ private ITask HookupHandlers(ITask task, bool filesystemChangesExpected)
                     //Logger.Trace("Ended Operation - Clearing Busy Flag");
                     IsBusy = false;
                 }
+            };
+            task.Catch(_ =>
+            {
+                if (filesystemChangesExpected)
+                {
+                    //Logger.Trace("Ended Operation - Enable Watcher");
+                    watcher.Start();
+                }
+
+                if (isExclusive)
+                {
+                    //Logger.Trace("Ended Operation - Clearing Busy Flag");
+                    IsBusy = false;
+                }
+
             });
             return task;
         }
diff --git a/src/GitHub.Api/Installer/UnzipTask.cs b/src/GitHub.Api/Installer/UnzipTask.cs
@@ -38,7 +38,6 @@ public override NPath RunWithReturn(bool success)
             }
             catch (Exception ex)
             {
-                Errors = ex.Message;
                 if (!RaiseFaultHandlers(ex))
                     throw;
             }
diff --git a/src/GitHub.Api/Tasks/ActionTask.cs b/src/GitHub.Api/Tasks/ActionTask.cs
@@ -10,7 +10,7 @@ class TaskQueue : TaskBase
         private TaskCompletionSource<bool> aggregateTask = new TaskCompletionSource<bool>();
         private readonly List<ITask> queuedTasks = new List<ITask>();
         private volatile bool isSuccessful = true;
-        private volatile Exception exception;
+        private volatile Exception taskException;
         private int finishedTaskCount;
 
         public TaskQueue() : base()
@@ -21,6 +21,7 @@ public TaskQueue() : base()
         public ITask Queue(ITask task)
         {
             task.OnEnd += TaskFinished;
+            task.Catch(e => TaskFinished(task, false, e));
             queuedTasks.Add(task);
             return this;
         }
@@ -37,7 +38,7 @@ private void TaskFinished(ITask task, bool success, Exception ex)
             if (!success)
             {
                 isSuccessful = false;
-                exception = ex;
+                taskException = ex;
             }
             var count = Interlocked.Increment(ref finishedTaskCount);
             if (count == queuedTasks.Count)
@@ -48,7 +49,7 @@ private void TaskFinished(ITask task, bool success, Exception ex)
                 }
                 else
                 {
-                    aggregateTask.TrySetException(exception);
+                    aggregateTask.TrySetException(taskException.GetBaseException());
                 }
             }
         }
@@ -105,7 +106,6 @@ public override void Run(bool success)
             }
             catch (Exception ex)
             {
-                Errors = ex.Message;
                 if (!RaiseFaultHandlers(ex))
                     throw;
             }
@@ -207,7 +207,6 @@ protected virtual void Run(bool success, T previousResult)
             }
             catch (Exception ex)
             {
-                Errors = ex.Message;
                 if (!RaiseFaultHandlers(ex))
                     throw;
             }
@@ -275,7 +274,6 @@ public override T RunWithReturn(bool success)
             }
             catch (Exception ex)
             {
-                Errors = ex.Message;
                 if (!RaiseFaultHandlers(ex))
                     throw;
             }
@@ -336,7 +334,6 @@ protected override TResult RunWithData(bool success, T previousResult)
             }
             catch (Exception ex)
             {
-                Errors = ex.Message;
                 if (!RaiseFaultHandlers(ex))
                     throw;
             }
@@ -402,16 +399,8 @@ public override List<T> RunWithReturn(bool success)
                     result = CallbackWithException(success, thrown);
                 }
             }
-            catch (AggregateException ex)
-            {
-                var e = ex.GetBaseException();
-                Errors = e.Message;
-                if (!RaiseFaultHandlers(e))
-                    throw e;
-            }
             catch (Exception ex)
             {
-                Errors = ex.Message;
                 if (!RaiseFaultHandlers(ex))
                     throw;
             }
@@ -469,7 +458,6 @@ protected override List<TResult> RunWithData(bool success, T previousResult)
             }
             catch (Exception ex)
             {
-                Errors = ex.Message;
                 if (!RaiseFaultHandlers(ex))
                     throw;
             }
@@ -481,4 +469,4 @@ protected override List<TResult> RunWithData(bool success, T previousResult)
             return result;
         }
     }
-}
+}
diff --git a/src/GitHub.Api/Tasks/DownloadTask.cs b/src/GitHub.Api/Tasks/DownloadTask.cs
@@ -61,7 +61,6 @@ public override NPath RunWithReturn(bool success)
             }
             catch (Exception ex)
             {
-                Errors = ex.Message;
                 if (!RaiseFaultHandlers(ex))
                     throw;
             }
diff --git a/src/GitHub.Api/Tasks/TaskBase.cs b/src/GitHub.Api/Tasks/TaskBase.cs
@@ -158,10 +158,11 @@ protected void Initialize(Task task)
                         Token.ThrowIfCancellationRequested();
                         tk.RunSynchronously(scheduler);
                     }
+                    else
+                        tk.Wait();
                 }
                 catch (Exception ex)
                 {
-                    Errors = ex.Message;
                     if (!RaiseFaultHandlers(ex))
                         throw;
                     Token.ThrowIfCancellationRequested();
@@ -245,11 +246,18 @@ public ITask Catch(Action<Exception> handler)
         public ITask Catch(Func<Exception, bool> handler)
         {
             Guard.ArgumentNotNull(handler, "handler");
-            catchHandler += handler;
+            CatchInternal(handler);
             DependsOn?.Catch(handler);
             return this;
         }
 
+        internal ITask CatchInternal(Func<Exception, bool> handler)
+        {
+            Guard.ArgumentNotNull(handler, "handler");
+            catchHandler += handler;
+            return this;
+        }
+
         /// <summary>
         /// Run a callback at the end of the task execution, on the same thread as the task that just finished, regardless of execution state
         /// This will always run on the same thread as the previous task
@@ -268,7 +276,14 @@ public ITask Finally(Action<bool> handler)
         public ITask Finally(Action<bool, Exception> actionToContinueWith, TaskAffinity affinity = TaskAffinity.Concurrent)
         {
             Guard.ArgumentNotNull(actionToContinueWith, nameof(actionToContinueWith));
-            return Finally(new ActionTask(Token, actionToContinueWith) { Affinity = affinity, Name = "Finally" });
+            return Then(new ActionTask(Token, (s, ex) =>
+                {
+                    actionToContinueWith(s, ex);
+                    if (!s)
+                        throw ex;
+                })
+                { Affinity = affinity, Name = "Finally" }, TaskRunOptions.OnAlways)
+                .CatchInternal(_ => true);
         }
 
         /// <summary>
@@ -438,13 +453,15 @@ protected virtual void RaiseOnStart()
 
         protected virtual bool RaiseFaultHandlers(Exception ex)
         {
+            exception = ex is AggregateException ? ex.GetBaseException() : ex;
+            Errors = exception.Message;
             taskFailed = true;
-            exception = ex;
             if (catchHandler == null)
                 return false;
+            var args = new object[] { exception };
             foreach (var handler in catchHandler.GetInvocationList())
             {
-                if ((bool)handler.DynamicInvoke(new object[] { ex }))
+                if ((bool)handler.DynamicInvoke(args))
                 {
                     exceptionWasHandled = true;
                     break;
@@ -497,15 +514,14 @@ protected virtual void CallFinallyHandler()
 
         protected Exception GetThrownException()
         {
-            if (DependsOn == null)
-                return null;
-
-            if (DependsOn.Task.Status == TaskStatus.Faulted)
+            var depends = DependsOn;
+            while (depends != null)
             {
-                var ex = DependsOn.Task.Exception;
-                return ex?.InnerException ?? ex;
+                if (depends.taskFailed)
+                    return depends.exception;
+                depends = depends.DependsOn;
             }
-            return DependsOn.GetThrownException();
+            return null;
         }
 
         public void UpdateProgress(long value, long total, string message = null)
@@ -583,7 +599,6 @@ protected void Initialize(Task<TResult> task)
                 }
                 catch (Exception ex)
                 {
-                    Errors = ex.Message;
                     if (!RaiseFaultHandlers(ex))
                         throw;
                     Token.ThrowIfCancellationRequested();
@@ -635,7 +650,7 @@ public override T Then<T>(T continuation, TaskRunOptions runOptions = TaskRunOpt
         public new ITask<TResult> Catch(Func<Exception, bool> handler)
         {
             Guard.ArgumentNotNull(handler, "handler");
-            catchHandler += handler;
+            CatchInternal(handler);
             DependsOn?.Catch(handler);
             return this;
         }
@@ -667,7 +682,14 @@ public ITask<TResult> Finally(Func<bool, Exception, TResult, TResult> continuati
         public ITask Finally(Action<bool, Exception, TResult> continuation, TaskAffinity affinity = TaskAffinity.Concurrent)
         {
             Guard.ArgumentNotNull(continuation, "continuation");
-            return Then(new ActionTask<TResult>(Token, continuation) { Affinity = affinity, Name = "Finally" }, TaskRunOptions.OnAlways);
+            return Then(new ActionTask<TResult>(Token, (s, ex, res) =>
+                {
+                    continuation(s, ex, res);
+                    if (!s)
+                        throw ex;
+                })
+                { Affinity = affinity, Name = "Finally" }, TaskRunOptions.OnAlways)
+                .CatchInternal(_ => true);
         }
 
         public new ITask<TResult> Start()
diff --git a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/ProjectWindowInterface.cs b/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/ProjectWindowInterface.cs
@@ -146,14 +146,9 @@ private static ITask CreateLockObjectTask(Object selected)
             NPath assetPath = AssetDatabase.GetAssetPath(selected.GetInstanceID()).ToNPath();
             NPath repositoryPath = manager.Environment.GetRepositoryPath(assetPath);
 
-            return Repository.RequestLock(repositoryPath)
-                      .FinallyInUI((success, ex) =>
-                      {
-                          if (success)
-                          {
-                              manager.TaskManager.Run(manager.UsageTracker.IncrementUnityProjectViewContextLfsLock, null);
-                          }
-                      });
+            var task = Repository.RequestLock(repositoryPath);
+            task.OnEnd += (_, s, ___) => { if (s) manager.TaskManager.Run(manager.UsageTracker.IncrementUnityProjectViewContextLfsLock, null); };
+            return task;
         }
 
         [MenuItem(AssetsMenuReleaseLock, true, 1000)]
diff --git a/src/tests/TaskSystemIntegrationTests/Tests.cs b/src/tests/TaskSystemIntegrationTests/Tests.cs
@@ -915,7 +915,7 @@ public async Task RunningDifferentTasksDependingOnPreviousResult()
             var callOrder = new List<string>();
 
             var taskEnd = new ActionTask(Token, () => callOrder.Add("chain completed")) { Name = "Chain Completed" };
-            var final = taskEnd.Finally((_, __) => { }, TaskAffinity.Concurrent);
+            var final = taskEnd.Finally((_, __) => { }, TaskAffinity.Exclusive);
 
             var taskStart = new FuncTask<bool>(Token, _ =>
                 {
@@ -942,12 +942,13 @@ public async Task RunningDifferentTasksDependingOnPreviousResult()
 
             await final.StartAndSwallowException();
 
-            CollectionAssert.AreEqual(new string[] {
+
+            Assert.AreEqual(new string[] {
                 "chain start",
                 "failing",
                 "on failure",
                 "chain completed"
-            }, callOrder);
+            }.Join(","), callOrder.Join(","));
         }
 
         private T LogAndReturnResult<T>(List<string> callOrder, string msg, T result)

Original file line number	Diff line number	Diff line change
`@@ -460,7 +460,7 @@ private ITask HookupHandlers(ITask task, bool filesystemChangesExpected)`
`460`	`460`	`}`
`461`	`461`	`};`
`462`	`462`
`463`		`- task.Finally(success =>`
	`463`	`+ task.OnEnd += (_, __, ___) =>`
`464`	`464`	`{`
`465`	`465`	`if (filesystemChangesExpected)`
`466`	`466`	`{`
`@@ -473,6 +473,21 @@ private ITask HookupHandlers(ITask task, bool filesystemChangesExpected)`
`473`	`473`	`//Logger.Trace("Ended Operation - Clearing Busy Flag");`
`474`	`474`	`IsBusy = false;`
`475`	`475`	`}`
	`476`	`+ };`
	`477`	`+ task.Catch(_ =>`
	`478`	`+ {`
	`479`	`+ if (filesystemChangesExpected)`
	`480`	`+ {`
	`481`	`+ //Logger.Trace("Ended Operation - Enable Watcher");`
	`482`	`+ watcher.Start();`
	`483`	`+ }`
	`484`	`+`
	`485`	`+ if (isExclusive)`
	`486`	`+ {`
	`487`	`+ //Logger.Trace("Ended Operation - Clearing Busy Flag");`
	`488`	`+ IsBusy = false;`
	`489`	`+ }`
	`490`	`+`
`476`	`491`	`});`
`477`	`492`	`return task;`
`478`	`493`	`}`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,6 @@ public override NPath RunWithReturn(bool success)`
`38`	`38`	`}`
`39`	`39`	`catch (Exception ex)`
`40`	`40`	`{`
`41`		`- Errors = ex.Message;`
`42`	`41`	`if (!RaiseFaultHandlers(ex))`
`43`	`42`	`throw;`
`44`	`43`	`}`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ class TaskQueue : TaskBase`
`10`	`10`	`private TaskCompletionSource<bool> aggregateTask = new TaskCompletionSource<bool>();`
`11`	`11`	`private readonly List<ITask> queuedTasks = new List<ITask>();`
`12`	`12`	`private volatile bool isSuccessful = true;`
`13`		`- private volatile Exception exception;`
	`13`	`+ private volatile Exception taskException;`
`14`	`14`	`private int finishedTaskCount;`
`15`	`15`
`16`	`16`	`public TaskQueue() : base()`
`@@ -21,6 +21,7 @@ public TaskQueue() : base()`
`21`	`21`	`public ITask Queue(ITask task)`
`22`	`22`	`{`
`23`	`23`	`task.OnEnd += TaskFinished;`
	`24`	`+ task.Catch(e => TaskFinished(task, false, e));`
`24`	`25`	`queuedTasks.Add(task);`
`25`	`26`	`return this;`
`26`	`27`	`}`
`@@ -37,7 +38,7 @@ private void TaskFinished(ITask task, bool success, Exception ex)`
`37`	`38`	`if (!success)`
`38`	`39`	`{`
`39`	`40`	`isSuccessful = false;`
`40`		`- exception = ex;`
	`41`	`+ taskException = ex;`
`41`	`42`	`}`
`42`	`43`	`var count = Interlocked.Increment(ref finishedTaskCount);`
`43`	`44`	`if (count == queuedTasks.Count)`
`@@ -48,7 +49,7 @@ private void TaskFinished(ITask task, bool success, Exception ex)`
`48`	`49`	`}`
`49`	`50`	`else`
`50`	`51`	`{`
`51`		`- aggregateTask.TrySetException(exception);`
	`52`	`+ aggregateTask.TrySetException(taskException.GetBaseException());`
`52`	`53`	`}`
`53`	`54`	`}`
`54`	`55`	`}`
`@@ -105,7 +106,6 @@ public override void Run(bool success)`
`105`	`106`	`}`
`106`	`107`	`catch (Exception ex)`
`107`	`108`	`{`
`108`		`- Errors = ex.Message;`
`109`	`109`	`if (!RaiseFaultHandlers(ex))`
`110`	`110`	`throw;`
`111`	`111`	`}`
`@@ -207,7 +207,6 @@ protected virtual void Run(bool success, T previousResult)`
`207`	`207`	`}`
`208`	`208`	`catch (Exception ex)`
`209`	`209`	`{`
`210`		`- Errors = ex.Message;`
`211`	`210`	`if (!RaiseFaultHandlers(ex))`
`212`	`211`	`throw;`
`213`	`212`	`}`
`@@ -275,7 +274,6 @@ public override T RunWithReturn(bool success)`
`275`	`274`	`}`
`276`	`275`	`catch (Exception ex)`
`277`	`276`	`{`
`278`		`- Errors = ex.Message;`
`279`	`277`	`if (!RaiseFaultHandlers(ex))`
`280`	`278`	`throw;`
`281`	`279`	`}`
`@@ -336,7 +334,6 @@ protected override TResult RunWithData(bool success, T previousResult)`
`336`	`334`	`}`
`337`	`335`	`catch (Exception ex)`
`338`	`336`	`{`
`339`		`- Errors = ex.Message;`
`340`	`337`	`if (!RaiseFaultHandlers(ex))`
`341`	`338`	`throw;`
`342`	`339`	`}`
`@@ -402,16 +399,8 @@ public override List<T> RunWithReturn(bool success)`
`402`	`399`	`result = CallbackWithException(success, thrown);`
`403`	`400`	`}`
`404`	`401`	`}`
`405`		`- catch (AggregateException ex)`
`406`		`- {`
`407`		`- var e = ex.GetBaseException();`
`408`		`- Errors = e.Message;`
`409`		`- if (!RaiseFaultHandlers(e))`
`410`		`- throw e;`
`411`		`- }`
`412`	`402`	`catch (Exception ex)`
`413`	`403`	`{`
`414`		`- Errors = ex.Message;`
`415`	`404`	`if (!RaiseFaultHandlers(ex))`
`416`	`405`	`throw;`
`417`	`406`	`}`
`@@ -469,7 +458,6 @@ protected override List<TResult> RunWithData(bool success, T previousResult)`
`469`	`458`	`}`
`470`	`459`	`catch (Exception ex)`
`471`	`460`	`{`
`472`		`- Errors = ex.Message;`
`473`	`461`	`if (!RaiseFaultHandlers(ex))`
`474`	`462`	`throw;`
`475`	`463`	`}`
`@@ -481,4 +469,4 @@ protected override List<TResult> RunWithData(bool success, T previousResult)`
`481`	`469`	`return result;`
`482`	`470`	`}`
`483`	`471`	`}`
`484`		`-}`
	`472`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,6 @@ public override NPath RunWithReturn(bool success)`
`61`	`61`	`}`
`62`	`62`	`catch (Exception ex)`
`63`	`63`	`{`
`64`		`- Errors = ex.Message;`
`65`	`64`	`if (!RaiseFaultHandlers(ex))`
`66`	`65`	`throw;`
`67`	`66`	`}`