From 7075338e685cfe7e0081d61fe8d050f96b4a60fe Mon Sep 17 00:00:00 2001 From: Matthias Dittrich Date: Sat, 24 Jun 2017 20:51:31 +0200 Subject: [PATCH 1/8] don't hide potentially important exceptions when cancelling the workflow. --- src/fsharp/FSharp.Core/control.fs | 26 +++++++-- .../Microsoft.FSharp.Control/Cancellation.fs | 56 +++++++++++++++++++ 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/src/fsharp/FSharp.Core/control.fs b/src/fsharp/FSharp.Core/control.fs index a614813e4ba..6fc33ddbc42 100644 --- a/src/fsharp/FSharp.Core/control.fs +++ b/src/fsharp/FSharp.Core/control.fs @@ -529,6 +529,14 @@ namespace Microsoft.FSharp.Control // delayPrim = "bindA (return ()) f" let delayA f = callA f () + let protectExn (cexn:OperationCanceledException) (edi:ExceptionDispatchInfo) = + let exn = edi.GetAssociatedSourceException() + let collected = + match cexn.InnerException with + | null -> [|exn|] + | :? AggregateException as a -> Array.append (a.InnerExceptions |> Seq.toArray) [|exn|] + | _ -> [|cexn.InnerException; exn|] + new OperationCanceledException(cexn.Message, new AggregateException(collected)) // Call p but augment the normal, exception and cancel continuations with a call to finallyFunction. // If the finallyFunction raises an exception then call the original exception continuation // with the new exception. If exception is raised after a cancellation, exception is ignored @@ -546,8 +554,8 @@ namespace Microsoft.FSharp.Control // If an exception is thrown we continue with the previous exception continuation. let econt exn = protect trampolineHolder args.aux.econt finallyFunction () (fun () -> args.aux.econt exn) // The cancellation continuation runs the finallyFunction and then runs the previous cancellation continuation. - // If an exception is thrown we continue with the previous cancellation continuation (the exception is lost) - let ccont cexn = protect trampolineHolder (fun _ -> args.aux.ccont cexn) finallyFunction () (fun () -> args.aux.ccont cexn) + // If an exception is thrown we collect/protect it in the OperationCancelledException + let ccont cexn = protect trampolineHolder (protectExn cexn >> args.aux.ccont) finallyFunction () (fun () -> args.aux.ccont cexn) invokeA p { args with cont = cont; aux = { args.aux with econt = econt; ccont = ccont } }) // Re-route the exception continuation to call to catchFunction. If catchFunction or the new process fail @@ -566,7 +574,7 @@ namespace Microsoft.FSharp.Control /// Call the finallyFunction if the computation results in a cancellation let whenCancelledA (finallyFunction : OperationCanceledException -> unit) p = unprotectedPrimitive (fun ({ aux = aux } as args)-> - let ccont exn = protect aux.trampolineHolder (fun _ -> aux.ccont exn) finallyFunction exn (fun _ -> aux.ccont exn) + let ccont exn = protect aux.trampolineHolder (protectExn exn >> aux.ccont) finallyFunction exn (fun _ -> aux.ccont exn) invokeA p { args with aux = { aux with ccont = ccont } }) let getCancellationToken() = @@ -896,7 +904,11 @@ namespace Microsoft.FSharp.Control queueAsync token (fun res -> resultCell.RegisterResult(Ok(res),reuseThread=true)) - (fun edi -> resultCell.RegisterResult(Error(edi),reuseThread=true)) + (fun edi -> + let result = + if token.IsCancellationRequested then Canceled(protectExn (new OperationCanceledException()) edi) + else Error(edi) + resultCell.RegisterResult(result,reuseThread=true)) (fun exn -> resultCell.RegisterResult(Canceled(exn),reuseThread=true)) computation |> unfake @@ -927,7 +939,11 @@ namespace Microsoft.FSharp.Control token trampolineHolder (fun res -> resultCell.RegisterResult(Ok(res),reuseThread=true)) - (fun edi -> resultCell.RegisterResult(Error(edi),reuseThread=true)) + (fun edi -> + let result = + if token.IsCancellationRequested then Canceled(protectExn (new OperationCanceledException()) edi) + else Error(edi) + resultCell.RegisterResult(result,reuseThread=true)) (fun exn -> resultCell.RegisterResult(Canceled(exn),reuseThread=true)) computation) |> unfake diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs index 54720b29d7c..99fd6224269 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs @@ -306,3 +306,59 @@ type CancellationType() = Assert.IsFalse((r1a <> r1a')) Assert.IsTrue((r1a <> r1b)) Assert.IsTrue((r1a <> r2)) + + [] + member this.TestCancellationKeepsExceptionInfo() = + let cts = new CancellationTokenSource() + let ewh = new ManualResetEvent(false) + let msg = "Cleanup failure" + let a = async { + try ewh.Set() |> ignore + do! Async.Sleep 10000 + finally raise <| Exception msg} + async { + ewh.WaitOne() |> ignore + cts.Cancel() + } |> Async.Start + + try + Async.RunSynchronously(a, cancellationToken = cts.Token) + with + :? OperationCanceledException as o -> + match o.InnerException with + | :? AggregateException as a -> + Assert.AreEqual (1, a.InnerExceptions.Count) + match a.InnerException with + | e when not (isNull e) -> + Assert.AreEqual(msg, e.Message) + | _ -> reraise() + | _ -> reraise() + + [] + member this.TestCancellationKeepsExceptionInfoWithTryWith() = + let cts = new CancellationTokenSource() + let ewh = new ManualResetEvent(false) + let msg = "Cleanup failure" + let a = async { + try + try ewh.Set() |> ignore + do! Async.Sleep 10000 + finally raise <| Exception msg + with :? InvalidOperationException -> () } + async { + ewh.WaitOne() |> ignore + cts.Cancel() + } |> Async.Start + + try + Async.RunSynchronously(a, cancellationToken = cts.Token) + with + :? OperationCanceledException as o -> + match o.InnerException with + | :? AggregateException as a -> + Assert.AreEqual (1, a.InnerExceptions.Count) + match a.InnerException with + | e when not (isNull e) -> + Assert.AreEqual(msg, e.Message) + | _ -> reraise() + | _ -> reraise() From e742506cf805c1eb31f9cbc64f9d0308fa848dfe Mon Sep 17 00:00:00 2001 From: Matthias Dittrich Date: Sat, 24 Jun 2017 21:21:47 +0200 Subject: [PATCH 2/8] add more tests to check if StartAsTask and RunSynchronously behave the same way. --- .../Microsoft.FSharp.Control/Cancellation.fs | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs index 99fd6224269..dd94716eeac 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs @@ -334,6 +334,34 @@ type CancellationType() = | _ -> reraise() | _ -> reraise() + + [] + member this.TestCancellationKeepsExceptionInfoAsTask() = + let cts = new CancellationTokenSource() + let ewh = new ManualResetEvent(false) + let msg = "Cleanup failure" + let a = async { + try ewh.Set() |> ignore + do! Async.Sleep 10000 + finally raise <| Exception msg} + async { + ewh.WaitOne() |> ignore + cts.Cancel() + } |> Async.Start + + try + Async.StartAsTask(a, cancellationToken = cts.Token).GetAwaiter().GetResult() + with + :? OperationCanceledException as o -> + match o.InnerException with + | :? AggregateException as a -> + Assert.AreEqual (1, a.InnerExceptions.Count) + match a.InnerException with + | e when not (isNull e) -> + Assert.AreEqual(msg, e.Message) + | _ -> reraise() + | _ -> reraise() + [] member this.TestCancellationKeepsExceptionInfoWithTryWith() = let cts = new CancellationTokenSource() @@ -362,3 +390,32 @@ type CancellationType() = Assert.AreEqual(msg, e.Message) | _ -> reraise() | _ -> reraise() + + [] + member this.TestCancellationKeepsExceptionInfoWithTryWithAsTask() = + let cts = new CancellationTokenSource() + let ewh = new ManualResetEvent(false) + let msg = "Cleanup failure" + let a = async { + try + try ewh.Set() |> ignore + do! Async.Sleep 10000 + finally raise <| Exception msg + with :? InvalidOperationException -> () } + async { + ewh.WaitOne() |> ignore + cts.Cancel() + } |> Async.Start + + try + Async.StartAsTask(a, cancellationToken = cts.Token).GetAwaiter().GetResult() + with + :? OperationCanceledException as o -> + match o.InnerException with + | :? AggregateException as a -> + Assert.AreEqual (1, a.InnerExceptions.Count) + match a.InnerException with + | e when not (isNull e) -> + Assert.AreEqual(msg, e.Message) + | _ -> reraise() + | _ -> reraise() From b8a325e4892b8bd43b0378bb8a0954896efa6108 Mon Sep 17 00:00:00 2001 From: Matthias Dittrich Date: Sat, 24 Jun 2017 22:45:58 +0200 Subject: [PATCH 3/8] use AsyncTaskMethodBuilder to fix the new tests --- src/fsharp/FSharp.Core/control.fs | 12 +++++++++++- .../Microsoft.FSharp.Control/Cancellation.fs | 17 ++++++++++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/fsharp/FSharp.Core/control.fs b/src/fsharp/FSharp.Core/control.fs index 6fc33ddbc42..4e844cce2c3 100644 --- a/src/fsharp/FSharp.Core/control.fs +++ b/src/fsharp/FSharp.Core/control.fs @@ -985,9 +985,15 @@ namespace Microsoft.FSharp.Control member __.Proceed = not isStopped member __.Stop() = isStopped <- true - let StartAsTask (token:CancellationToken, computation : Async<_>,taskCreationOptions) : Task<_> = + let StartAsTask (token:CancellationToken, computation : Async<'a>,taskCreationOptions) : Task<'a> = +#if !FX_NO_ASYNCTASKMETHODBUILDER + // AsyncTaskMethodBuilder allows us to better control the cancellation process by setting custom exception objects. + let _ = defaultArg taskCreationOptions TaskCreationOptions.None + let tcs = System.Runtime.CompilerServices.AsyncTaskMethodBuilder<'a>() +#else let taskCreationOptions = defaultArg taskCreationOptions TaskCreationOptions.None let tcs = new TaskCompletionSource<_>(taskCreationOptions) +#endif // The contract: // a) cancellation signal should always propagate to the computation @@ -997,7 +1003,11 @@ namespace Microsoft.FSharp.Control token (fun r -> tcs.SetResult r |> fake) (fun edi -> tcs.SetException edi.SourceException |> fake) +#if !FX_NO_ASYNCTASKMETHODBUILDER + (fun exn -> tcs.SetException (new TaskCanceledException(exn.Message, exn.InnerException)) |> fake) +#else (fun _ -> tcs.SetCanceled() |> fake) +#endif computation |> unfake task diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs index dd94716eeac..02244ea7dcc 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs @@ -334,7 +334,6 @@ type CancellationType() = | _ -> reraise() | _ -> reraise() - [] member this.TestCancellationKeepsExceptionInfoAsTask() = let cts = new CancellationTokenSource() @@ -349,10 +348,11 @@ type CancellationType() = cts.Cancel() } |> Async.Start + let t = Async.StartAsTask(a, cancellationToken = cts.Token) try - Async.StartAsTask(a, cancellationToken = cts.Token).GetAwaiter().GetResult() + t.GetAwaiter().GetResult() with - :? OperationCanceledException as o -> + | :? OperationCanceledException as o -> match o.InnerException with | :? AggregateException as a -> Assert.AreEqual (1, a.InnerExceptions.Count) @@ -362,6 +362,9 @@ type CancellationType() = | _ -> reraise() | _ -> reraise() + Assert.IsTrue(t.IsCompleted, "Task should be marked as completed") + Assert.IsTrue(t.IsCanceled, "Task should be marked as cancelled") + [] member this.TestCancellationKeepsExceptionInfoWithTryWith() = let cts = new CancellationTokenSource() @@ -407,10 +410,11 @@ type CancellationType() = cts.Cancel() } |> Async.Start + let t = Async.StartAsTask(a, cancellationToken = cts.Token) try - Async.StartAsTask(a, cancellationToken = cts.Token).GetAwaiter().GetResult() + t.GetAwaiter().GetResult() with - :? OperationCanceledException as o -> + | :? OperationCanceledException as o -> match o.InnerException with | :? AggregateException as a -> Assert.AreEqual (1, a.InnerExceptions.Count) @@ -419,3 +423,6 @@ type CancellationType() = Assert.AreEqual(msg, e.Message) | _ -> reraise() | _ -> reraise() + + Assert.IsTrue(t.IsCompleted, "Task should be marked as completed") + Assert.IsTrue(t.IsCanceled, "Task should be marked as cancelled") From 304e5dc1122779663bb3903f6cde69478962d8f4 Mon Sep 17 00:00:00 2001 From: Matthias Dittrich Date: Sun, 25 Jun 2017 10:52:25 +0200 Subject: [PATCH 4/8] more tests, flatten exceptions and improve information flow on AwaitTask as well. --- src/fsharp/FSharp.Core/control.fs | 53 ++++++-- .../Microsoft.FSharp.Control/AsyncType.fs | 114 +++++++++++++++++- 2 files changed, 156 insertions(+), 11 deletions(-) diff --git a/src/fsharp/FSharp.Core/control.fs b/src/fsharp/FSharp.Core/control.fs index 4e844cce2c3..083c887539b 100644 --- a/src/fsharp/FSharp.Core/control.fs +++ b/src/fsharp/FSharp.Core/control.fs @@ -529,14 +529,28 @@ namespace Microsoft.FSharp.Control // delayPrim = "bindA (return ()) f" let delayA f = callA f () + // protect an exception in an cancellation workflow and adds it to the given OperationCanceledException + // ie creates a new instance, where the new information is added, + // if cexn is a TaskCanceledException the type is preserved as well. let protectExn (cexn:OperationCanceledException) (edi:ExceptionDispatchInfo) = + // It's probably ok to not care about the stack of the cexn object, because + // 1. we often don't even collect the stack in the ccont route + // 2. there are no suited APIs to handle this (at best we could add the original instance to the new instance...) + // If we ever need this we probably need to provide our own sub-types of OperationCanceledException + // and TaskCanceledException let exn = edi.GetAssociatedSourceException() let collected = match cexn.InnerException with | null -> [|exn|] - | :? AggregateException as a -> Array.append (a.InnerExceptions |> Seq.toArray) [|exn|] + | :? AggregateException as a -> Array.append (a.Flatten().InnerExceptions |> Seq.toArray) [|exn|] | _ -> [|cexn.InnerException; exn|] - new OperationCanceledException(cexn.Message, new AggregateException(collected)) + let aggr = (new AggregateException(collected)).Flatten() + match cexn with + | :? TaskCanceledException -> + new TaskCanceledException(cexn.Message, aggr) + :> OperationCanceledException + | _ -> + new OperationCanceledException(cexn.Message, aggr) // Call p but augment the normal, exception and cancel continuations with a call to finallyFunction. // If the finallyFunction raises an exception then call the original exception continuation // with the new exception. If exception is raised after a cancellation, exception is ignored @@ -1002,8 +1016,15 @@ namespace Microsoft.FSharp.Control queueAsync token (fun r -> tcs.SetResult r |> fake) - (fun edi -> tcs.SetException edi.SourceException |> fake) + (fun edi -> + let wrapper = + if token.IsCancellationRequested then + protectExn (new TaskCanceledException()) edi :> exn + else + edi.SourceException + tcs.SetException wrapper |> fake) #if !FX_NO_ASYNCTASKMETHODBUILDER + // We wrap in a TaskCanceledException to maintain backwards compat. (fun exn -> tcs.SetException (new TaskCanceledException(exn.Message, exn.InnerException)) |> fake) #else (fun _ -> tcs.SetCanceled() |> fake) @@ -1204,14 +1225,29 @@ namespace Microsoft.FSharp.Control // Contains helpers that will attach continuation to the given task. // Should be invoked as a part of protectedPrimitive(withResync) call module TaskHelpers = + // This uses a trick to get the underlying OperationCanceledException + let inline getCancelledException (completedTask:Task) (waitWithAwaiter) = + let fallback = new TaskCanceledException(completedTask) :> OperationCanceledException + // sadly there is no other public api to retrieve it, but to call .GetAwaiter().GetResult(). + try waitWithAwaiter() + // should not happen, but just in case... + fallback + with + | :? OperationCanceledException as o -> o + | other -> + // shouldn't happen, but just in case... + new TaskCanceledException(fallback.Message, other) :> OperationCanceledException + let continueWith (task : Task<'T>, args, useCcontForTaskCancellation) = let continuation (completedTask : Task<_>) : unit = args.aux.trampolineHolder.Protect((fun () -> if completedTask.IsCanceled then + let cancelledException = + getCancelledException completedTask (fun () -> completedTask.GetAwaiter().GetResult() |> ignore) if useCcontForTaskCancellation - then args.aux.ccont (new OperationCanceledException(args.aux.token)) - else args.aux.econt (ExceptionDispatchInfo.Capture(new TaskCanceledException(completedTask))) + then args.aux.ccont (cancelledException) + else args.aux.econt (ExceptionDispatchInfo.Capture(cancelledException)) elif completedTask.IsFaulted then args.aux.econt (MayLoseStackTrace(completedTask.Exception)) else @@ -1221,12 +1257,15 @@ namespace Microsoft.FSharp.Control let continueWithUnit (task : Task, args, useCcontForTaskCancellation) = + let continuation (completedTask : Task) : unit = args.aux.trampolineHolder.Protect((fun () -> if completedTask.IsCanceled then + let cancelledException = + getCancelledException completedTask (fun () -> completedTask.GetAwaiter().GetResult()) if useCcontForTaskCancellation - then args.aux.ccont (new OperationCanceledException(args.aux.token)) - else args.aux.econt (ExceptionDispatchInfo.Capture(new TaskCanceledException(completedTask))) + then args.aux.ccont (cancelledException) + else args.aux.econt (ExceptionDispatchInfo.Capture(cancelledException)) elif completedTask.IsFaulted then args.aux.econt (MayLoseStackTrace(completedTask.Exception)) else diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs index 04f1d1a7ff2..c9f8c7427a7 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs @@ -131,7 +131,12 @@ type AsyncType() = #if !FX_NO_TPL_PARALLEL member private this.WaitASec (t:Task) = - let result = t.Wait(TimeSpan(hours=0,minutes=0,seconds=1)) + let result = + try t.Wait(TimeSpan(hours=0,minutes=0,seconds=1)) + with :? AggregateException -> + // This throws the "original" exception + t.GetAwaiter().GetResult() + false Assert.IsTrue(result, "Task did not finish after waiting for a second.") @@ -173,11 +178,112 @@ type AsyncType() = try this.WaitASec t - with :? AggregateException as a -> - match a.InnerException with - | :? TaskCanceledException as t -> () + with :? TaskCanceledException -> () + Assert.IsTrue (t.IsCompleted, "Task is not completed") + Assert.IsTrue (t.IsCanceled, "Task is not cancelled") + + [] + member this.StartAsTaskCancellationViaException () = + let cts = new CancellationTokenSource() + let tcs = TaskCompletionSource() + let a = async { + cts.CancelAfter (100) + do! tcs.Task |> Async.AwaitTask } +#if FSCORE_PORTABLE_NEW || coreclr + let t : Task = +#else + use t : Task = +#endif + Async.StartAsTask(a, cancellationToken = cts.Token) + + // Should not finish + try + let result = t.Wait(300) + Assert.IsFalse (result) + with :? AggregateException -> Assert.Fail "Task should not finish, jet" + + let msg = "Custom non-conforming 3rd-Party-Api throws" + tcs.SetException(Exception msg) + + try + this.WaitASec t + with :? TaskCanceledException as t -> + match t.InnerException with + | :? AggregateException as a -> + Assert.AreEqual(1, a.InnerExceptions.Count) + Assert.AreEqual(msg, a.InnerException.Message) + | _ -> reraise() + Assert.IsTrue (t.IsCompleted, "Task is not completed") + Assert.IsTrue (t.IsCanceled, "Task is not cancelled") + + [] + member this.RunSynchronouslyCancellation () = + let cts = new CancellationTokenSource() + let tcs = TaskCompletionSource() + let a = async { + cts.CancelAfter (100) + do! tcs.Task |> Async.AwaitTask } +#if FSCORE_PORTABLE_NEW || coreclr + let t : Task = +#else + use t : Task = +#endif + Task.Run(new Func(fun () -> Async.RunSynchronously(a, cancellationToken = cts.Token))) + + // Should not finish + try + let result = t.Wait(300) + Assert.IsFalse (result) + with :? AggregateException -> Assert.Fail "Task should not finish, jet" + + tcs.SetCanceled() + + try + this.WaitASec t + with :? OperationCanceledException -> () + + Assert.IsTrue (t.IsCompleted, "Task is not completed") + // We used Task.Run for convenience, it will not notice the cancellation + // -> Cancellation is noticed by RunSynchronously throwing 'OperationCanceledException' + // which is tested above + //Assert.IsTrue (t.IsCanceled, "Task is not cancelled") + + [] + member this.RunSynchronouslyCancellationViaException () = + let cts = new CancellationTokenSource() + let tcs = TaskCompletionSource() + let a = async { + cts.CancelAfter (100) + do! tcs.Task |> Async.AwaitTask } +#if FSCORE_PORTABLE_NEW || coreclr + let t : Task = +#else + use t : Task = +#endif + Task.Run(new Func(fun () -> Async.RunSynchronously(a, cancellationToken = cts.Token))) + + // Should not finish + try + let result = t.Wait(300) + Assert.IsFalse (result) + with :? AggregateException -> Assert.Fail "Task should not finish, jet" + + let msg = "Custom non-conforming 3rd-Party-Api throws" + tcs.SetException(Exception msg) + + try + this.WaitASec t + with :? OperationCanceledException as t -> + match t.InnerException with + | :? AggregateException as a -> + Assert.AreEqual(1, a.InnerExceptions.Count) + Assert.AreEqual(msg, a.InnerException.Message) | _ -> reraise() Assert.IsTrue (t.IsCompleted, "Task is not completed") + // We used Task.Run for convenience, it will not notice the cancellation + // -> Cancellation is noticed by RunSynchronously throwing 'OperationCanceledException' + // which is tested above + //Assert.IsTrue (t.IsCanceled, "Task is not cancelled") [] member this.StartTask () = From a7d97e3c68003f64c3f190f11c3c4bd4c525132d Mon Sep 17 00:00:00 2001 From: Matthias Dittrich Date: Sun, 25 Jun 2017 11:02:02 +0200 Subject: [PATCH 5/8] cleanup code --- src/fsharp/FSharp.Core/control.fs | 9 +++------ .../Microsoft.FSharp.Control/Cancellation.fs | 12 ++++-------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/fsharp/FSharp.Core/control.fs b/src/fsharp/FSharp.Core/control.fs index 083c887539b..0e3078112d0 100644 --- a/src/fsharp/FSharp.Core/control.fs +++ b/src/fsharp/FSharp.Core/control.fs @@ -543,14 +543,11 @@ namespace Microsoft.FSharp.Control match cexn.InnerException with | null -> [|exn|] | :? AggregateException as a -> Array.append (a.Flatten().InnerExceptions |> Seq.toArray) [|exn|] - | _ -> [|cexn.InnerException; exn|] + | inner -> [|inner; exn|] let aggr = (new AggregateException(collected)).Flatten() match cexn with - | :? TaskCanceledException -> - new TaskCanceledException(cexn.Message, aggr) - :> OperationCanceledException - | _ -> - new OperationCanceledException(cexn.Message, aggr) + | :? TaskCanceledException -> new TaskCanceledException(cexn.Message, aggr) :> OperationCanceledException + | _ -> new OperationCanceledException(cexn.Message, aggr) // Call p but augment the normal, exception and cancel continuations with a call to finallyFunction. // If the finallyFunction raises an exception then call the original exception continuation // with the new exception. If exception is raised after a cancellation, exception is ignored diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs index 02244ea7dcc..74d56d523ff 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Cancellation.fs @@ -323,8 +323,7 @@ type CancellationType() = try Async.RunSynchronously(a, cancellationToken = cts.Token) - with - :? OperationCanceledException as o -> + with :? OperationCanceledException as o -> match o.InnerException with | :? AggregateException as a -> Assert.AreEqual (1, a.InnerExceptions.Count) @@ -351,8 +350,7 @@ type CancellationType() = let t = Async.StartAsTask(a, cancellationToken = cts.Token) try t.GetAwaiter().GetResult() - with - | :? OperationCanceledException as o -> + with :? OperationCanceledException as o -> match o.InnerException with | :? AggregateException as a -> Assert.AreEqual (1, a.InnerExceptions.Count) @@ -383,8 +381,7 @@ type CancellationType() = try Async.RunSynchronously(a, cancellationToken = cts.Token) - with - :? OperationCanceledException as o -> + with :? OperationCanceledException as o -> match o.InnerException with | :? AggregateException as a -> Assert.AreEqual (1, a.InnerExceptions.Count) @@ -413,8 +410,7 @@ type CancellationType() = let t = Async.StartAsTask(a, cancellationToken = cts.Token) try t.GetAwaiter().GetResult() - with - | :? OperationCanceledException as o -> + with :? OperationCanceledException as o -> match o.InnerException with | :? AggregateException as a -> Assert.AreEqual (1, a.InnerExceptions.Count) From d5881c91bf885bf91328496feef2eaadfdf8f6ac Mon Sep 17 00:00:00 2001 From: Matthias Dittrich Date: Fri, 9 Feb 2018 20:11:34 +0100 Subject: [PATCH 6/8] Switch branches. --- src/fsharp/FSharp.Core/control.fs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/fsharp/FSharp.Core/control.fs b/src/fsharp/FSharp.Core/control.fs index 0e3078112d0..925d2fb8ada 100644 --- a/src/fsharp/FSharp.Core/control.fs +++ b/src/fsharp/FSharp.Core/control.fs @@ -997,13 +997,13 @@ namespace Microsoft.FSharp.Control member __.Stop() = isStopped <- true let StartAsTask (token:CancellationToken, computation : Async<'a>,taskCreationOptions) : Task<'a> = -#if !FX_NO_ASYNCTASKMETHODBUILDER +#if FX_NO_ASYNCTASKMETHODBUILDER + let taskCreationOptions = defaultArg taskCreationOptions TaskCreationOptions.None + let tcs = new TaskCompletionSource<_>(taskCreationOptions) +#else // AsyncTaskMethodBuilder allows us to better control the cancellation process by setting custom exception objects. let _ = defaultArg taskCreationOptions TaskCreationOptions.None let tcs = System.Runtime.CompilerServices.AsyncTaskMethodBuilder<'a>() -#else - let taskCreationOptions = defaultArg taskCreationOptions TaskCreationOptions.None - let tcs = new TaskCompletionSource<_>(taskCreationOptions) #endif // The contract: @@ -1020,11 +1020,11 @@ namespace Microsoft.FSharp.Control else edi.SourceException tcs.SetException wrapper |> fake) -#if !FX_NO_ASYNCTASKMETHODBUILDER +#if FX_NO_ASYNCTASKMETHODBUILDER + (fun _ -> tcs.SetCanceled() |> fake) +#else // We wrap in a TaskCanceledException to maintain backwards compat. (fun exn -> tcs.SetException (new TaskCanceledException(exn.Message, exn.InnerException)) |> fake) -#else - (fun _ -> tcs.SetCanceled() |> fake) #endif computation |> unfake From ac0a9c11c75ef2af3bf5a7c83e667d9305103fea Mon Sep 17 00:00:00 2001 From: Matthias Dittrich Date: Sun, 18 Feb 2018 23:05:40 +0100 Subject: [PATCH 7/8] some more fixes and PR hints. --- src/fsharp/FSharp.Core/control.fs | 44 ++++++++++--------- .../Microsoft.FSharp.Control/AsyncType.fs | 8 ++-- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/fsharp/FSharp.Core/control.fs b/src/fsharp/FSharp.Core/control.fs index 925d2fb8ada..cb8cb247417 100644 --- a/src/fsharp/FSharp.Core/control.fs +++ b/src/fsharp/FSharp.Core/control.fs @@ -532,7 +532,7 @@ namespace Microsoft.FSharp.Control // protect an exception in an cancellation workflow and adds it to the given OperationCanceledException // ie creates a new instance, where the new information is added, // if cexn is a TaskCanceledException the type is preserved as well. - let protectExn (cexn:OperationCanceledException) (edi:ExceptionDispatchInfo) = + let augmentOperationCancelledException (cexn:OperationCanceledException) (edi:ExceptionDispatchInfo) = // It's probably ok to not care about the stack of the cexn object, because // 1. we often don't even collect the stack in the ccont route // 2. there are no suited APIs to handle this (at best we could add the original instance to the new instance...) @@ -566,7 +566,7 @@ namespace Microsoft.FSharp.Control let econt exn = protect trampolineHolder args.aux.econt finallyFunction () (fun () -> args.aux.econt exn) // The cancellation continuation runs the finallyFunction and then runs the previous cancellation continuation. // If an exception is thrown we collect/protect it in the OperationCancelledException - let ccont cexn = protect trampolineHolder (protectExn cexn >> args.aux.ccont) finallyFunction () (fun () -> args.aux.ccont cexn) + let ccont cexn = protect trampolineHolder (augmentOperationCancelledException cexn >> args.aux.ccont) finallyFunction () (fun () -> args.aux.ccont cexn) invokeA p { args with cont = cont; aux = { args.aux with econt = econt; ccont = ccont } }) // Re-route the exception continuation to call to catchFunction. If catchFunction or the new process fail @@ -585,7 +585,7 @@ namespace Microsoft.FSharp.Control /// Call the finallyFunction if the computation results in a cancellation let whenCancelledA (finallyFunction : OperationCanceledException -> unit) p = unprotectedPrimitive (fun ({ aux = aux } as args)-> - let ccont exn = protect aux.trampolineHolder (protectExn exn >> aux.ccont) finallyFunction exn (fun _ -> aux.ccont exn) + let ccont exn = protect aux.trampolineHolder (augmentOperationCancelledException exn >> aux.ccont) finallyFunction exn (fun _ -> aux.ccont exn) invokeA p { args with aux = { aux with ccont = ccont } }) let getCancellationToken() = @@ -917,7 +917,7 @@ namespace Microsoft.FSharp.Control (fun res -> resultCell.RegisterResult(Ok(res),reuseThread=true)) (fun edi -> let result = - if token.IsCancellationRequested then Canceled(protectExn (new OperationCanceledException()) edi) + if token.IsCancellationRequested then Canceled(augmentOperationCancelledException (new OperationCanceledException()) edi) else Error(edi) resultCell.RegisterResult(result,reuseThread=true)) (fun exn -> resultCell.RegisterResult(Canceled(exn),reuseThread=true)) @@ -952,7 +952,7 @@ namespace Microsoft.FSharp.Control (fun res -> resultCell.RegisterResult(Ok(res),reuseThread=true)) (fun edi -> let result = - if token.IsCancellationRequested then Canceled(protectExn (new OperationCanceledException()) edi) + if token.IsCancellationRequested then Canceled(augmentOperationCancelledException (new OperationCanceledException()) edi) else Error(edi) resultCell.RegisterResult(result,reuseThread=true)) (fun exn -> resultCell.RegisterResult(Canceled(exn),reuseThread=true)) @@ -1016,7 +1016,7 @@ namespace Microsoft.FSharp.Control (fun edi -> let wrapper = if token.IsCancellationRequested then - protectExn (new TaskCanceledException()) edi :> exn + augmentOperationCancelledException (new TaskCanceledException()) edi :> exn else edi.SourceException tcs.SetException wrapper |> fake) @@ -1224,27 +1224,28 @@ namespace Microsoft.FSharp.Control module TaskHelpers = // This uses a trick to get the underlying OperationCanceledException let inline getCancelledException (completedTask:Task) (waitWithAwaiter) = - let fallback = new TaskCanceledException(completedTask) :> OperationCanceledException + let fallback() = new TaskCanceledException(completedTask) :> OperationCanceledException // sadly there is no other public api to retrieve it, but to call .GetAwaiter().GetResult(). try waitWithAwaiter() // should not happen, but just in case... - fallback + fallback() with | :? OperationCanceledException as o -> o | other -> // shouldn't happen, but just in case... - new TaskCanceledException(fallback.Message, other) :> OperationCanceledException + new TaskCanceledException(fallback().Message, other) :> OperationCanceledException - let continueWith (task : Task<'T>, args, useCcontForTaskCancellation) = + let continueWith (task : Task<'T>, args, useSimpleCcontForTaskCancellationAndLooseException) = let continuation (completedTask : Task<_>) : unit = args.aux.trampolineHolder.Protect((fun () -> if completedTask.IsCanceled then - let cancelledException = - getCancelledException completedTask (fun () -> completedTask.GetAwaiter().GetResult() |> ignore) - if useCcontForTaskCancellation - then args.aux.ccont (cancelledException) - else args.aux.econt (ExceptionDispatchInfo.Capture(cancelledException)) + if useSimpleCcontForTaskCancellationAndLooseException + then args.aux.ccont (new OperationCanceledException(args.aux.token)) + else + let cancelledException = + getCancelledException completedTask (fun () -> completedTask.GetAwaiter().GetResult() |> ignore) + args.aux.econt (ExceptionDispatchInfo.Capture(cancelledException)) elif completedTask.IsFaulted then args.aux.econt (MayLoseStackTrace(completedTask.Exception)) else @@ -1252,17 +1253,18 @@ namespace Microsoft.FSharp.Control task.ContinueWith(Action>(continuation)) |> ignore |> fake - let continueWithUnit (task : Task, args, useCcontForTaskCancellation) = + let continueWithUnit (task : Task, args, useSimpleCcontForTaskCancellationAndLooseException) = let continuation (completedTask : Task) : unit = args.aux.trampolineHolder.Protect((fun () -> if completedTask.IsCanceled then - let cancelledException = - getCancelledException completedTask (fun () -> completedTask.GetAwaiter().GetResult()) - if useCcontForTaskCancellation - then args.aux.ccont (cancelledException) - else args.aux.econt (ExceptionDispatchInfo.Capture(cancelledException)) + if useSimpleCcontForTaskCancellationAndLooseException + then args.aux.ccont (new OperationCanceledException(args.aux.token)) + else + let cancelledException = + getCancelledException completedTask (fun () -> completedTask.GetAwaiter().GetResult()) + args.aux.econt (ExceptionDispatchInfo.Capture(cancelledException)) elif completedTask.IsFaulted then args.aux.econt (MayLoseStackTrace(completedTask.Exception)) else diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs index c9f8c7427a7..79843147de1 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs @@ -172,7 +172,7 @@ type AsyncType() = try let result = t.Wait(300) Assert.IsFalse (result) - with :? AggregateException -> Assert.Fail "Task should not finish, jet" + with :? AggregateException -> Assert.Fail "Task should not finish, yet" tcs.SetCanceled() @@ -200,7 +200,7 @@ type AsyncType() = try let result = t.Wait(300) Assert.IsFalse (result) - with :? AggregateException -> Assert.Fail "Task should not finish, jet" + with :? AggregateException -> Assert.Fail "Task should not finish, yet" let msg = "Custom non-conforming 3rd-Party-Api throws" tcs.SetException(Exception msg) @@ -234,7 +234,7 @@ type AsyncType() = try let result = t.Wait(300) Assert.IsFalse (result) - with :? AggregateException -> Assert.Fail "Task should not finish, jet" + with :? AggregateException -> Assert.Fail "Task should not finish, yet" tcs.SetCanceled() @@ -266,7 +266,7 @@ type AsyncType() = try let result = t.Wait(300) Assert.IsFalse (result) - with :? AggregateException -> Assert.Fail "Task should not finish, jet" + with :? AggregateException -> Assert.Fail "Task should not finish, yet" let msg = "Custom non-conforming 3rd-Party-Api throws" tcs.SetException(Exception msg) From 49c26bf91f1727fc65f82fb86c79672c4e8839e3 Mon Sep 17 00:00:00 2001 From: Matthias Dittrich Date: Thu, 22 Feb 2018 18:22:35 +0100 Subject: [PATCH 8/8] remove FX_NO_ASYNCTASKMETHODBUILDER --- src/fsharp/FSharp.Core/control.fs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/fsharp/FSharp.Core/control.fs b/src/fsharp/FSharp.Core/control.fs index cb8cb247417..159ac4c44dc 100644 --- a/src/fsharp/FSharp.Core/control.fs +++ b/src/fsharp/FSharp.Core/control.fs @@ -1020,12 +1020,8 @@ namespace Microsoft.FSharp.Control else edi.SourceException tcs.SetException wrapper |> fake) -#if FX_NO_ASYNCTASKMETHODBUILDER - (fun _ -> tcs.SetCanceled() |> fake) -#else // We wrap in a TaskCanceledException to maintain backwards compat. (fun exn -> tcs.SetException (new TaskCanceledException(exn.Message, exn.InnerException)) |> fake) -#endif computation |> unfake task