From e6945e2c7dd0b681f35733a1468077537a5ae265 Mon Sep 17 00:00:00 2001 From: Fraser Waters Date: Mon, 5 Aug 2019 23:02:36 +0100 Subject: [PATCH 1/3] Cancel task continuations when async contexts are cancelled Currently if you transform a task into an async via Async.AwaitTask and then run that async the continuation added to the underlying task is only ever resolved if the task itself resolved (either via completion, exception, or cancellation). Notably if the async created by Async.AwaitTask is cancelled the task continuation isn't. We're seeing this manifest as a large memory leak when we repeatedly run Async.Choice with a task that only resolves if an error elsewhere in the system is hit. Each time we run Async.Choice it appends another task continuation, the async is then cancelled (so if the task did resolve the async continuation would immediately see it was cancelled anyway) but critically the task continuations are not cancelled and so stay allocated. This commit changes the task continuation to now cancel if the asyncs cancellation token requests cancellation. Note that we need to have a second continuation to watch for the case that the first continuation is cancelled and thus never run, the second continuation then triggers the async cancellation or exception continuation as appropriate. This also means that some asyncs will now report cancelled sooner (see the changes to the StartAsTaskCancellation test) while before they would of been blocked waiting for a task to complete before any of their continuations could run. --- src/fsharp/FSharp.Core/async.fs | 28 +++++++++++++++++-- .../Microsoft.FSharp.Control/AsyncType.fs | 24 ++++++++++++++-- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/fsharp/FSharp.Core/async.fs b/src/fsharp/FSharp.Core/async.fs index 1f20f7c8110..5591f486984 100644 --- a/src/fsharp/FSharp.Core/async.fs +++ b/src/fsharp/FSharp.Core/async.fs @@ -984,7 +984,19 @@ namespace Microsoft.FSharp.Control else ctxt.cont completedTask.Result) |> unfake - task.ContinueWith(Action>(continuation)) |> ignore |> fake + let cancelContinuation (cancelledTask: Task) : unit = + ctxt.trampolineHolder.ExecuteWithTrampoline (fun () -> + if useCcontForTaskCancellation then + ctxt.OnCancellation () + else + let edi = ExceptionDispatchInfo.Capture(TaskCanceledException cancelledTask) + ctxt.CallExceptionContinuation edi + ) |> unfake + + task + .ContinueWith(Action>(continuation), ctxt.token) + .ContinueWith(Action(cancelContinuation), TaskContinuationOptions.OnlyOnCanceled) + |> ignore |> fake [] let taskContinueWithUnit (task: Task) (ctxt: AsyncActivation) useCcontForTaskCancellation = @@ -1003,7 +1015,19 @@ namespace Microsoft.FSharp.Control else ctxt.cont ()) |> unfake - task.ContinueWith(Action(continuation)) |> ignore |> fake + let cancelContinuation (cancelledTask: Task) : unit = + ctxt.trampolineHolder.ExecuteWithTrampoline (fun () -> + if useCcontForTaskCancellation then + ctxt.OnCancellation () + else + let edi = ExceptionDispatchInfo.Capture(new TaskCanceledException(cancelledTask)) + ctxt.CallExceptionContinuation edi + ) |> unfake + + task + .ContinueWith(Action(continuation), ctxt.token) + .ContinueWith(Action(cancelContinuation), TaskContinuationOptions.OnlyOnCanceled) + |> ignore |> fake [] type AsyncIAsyncResult<'T>(callback: System.AsyncCallback, state:obj) = 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 c0797038f56..5e763ed1134 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 @@ -143,7 +143,7 @@ type AsyncType() = Assert.AreEqual(s, t.Result) [] - member this.StartAsTaskCancellation () = + member this.StartAsTaskCancelAsync () = let cts = new CancellationTokenSource() let tcs = TaskCompletionSource() let a = async { @@ -155,12 +155,32 @@ type AsyncType() = use t : Task = #endif Async.StartAsTask(a, cancellationToken = cts.Token) + + try + this.WaitASec t + with :? AggregateException as a -> + match a.InnerException with + | :? TaskCanceledException as t -> () + | _ -> reraise() + Assert.IsTrue (t.IsCompleted, "Task is not completed") + + [] + member this.StartAsTaskCancelTask () = + let tcs = TaskCompletionSource() + let a = async { + do! tcs.Task |> Async.AwaitTask } +#if !NET46 + let t : Task = +#else + use t : Task = +#endif + Async.StartAsTask(a) // Should not finish 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() From 5fb9fb07de59c1dd60c61dbae75545994ae522cf Mon Sep 17 00:00:00 2001 From: Fraser Waters Date: Sat, 10 Aug 2019 11:48:21 +0100 Subject: [PATCH 2/3] Use cancellation continuation if the async is cancelled --- src/fsharp/FSharp.Core/async.fs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/fsharp/FSharp.Core/async.fs b/src/fsharp/FSharp.Core/async.fs index 5591f486984..6a225de0872 100644 --- a/src/fsharp/FSharp.Core/async.fs +++ b/src/fsharp/FSharp.Core/async.fs @@ -984,13 +984,9 @@ namespace Microsoft.FSharp.Control else ctxt.cont completedTask.Result) |> unfake - let cancelContinuation (cancelledTask: Task) : unit = + let cancelContinuation (_: Task) : unit = ctxt.trampolineHolder.ExecuteWithTrampoline (fun () -> - if useCcontForTaskCancellation then - ctxt.OnCancellation () - else - let edi = ExceptionDispatchInfo.Capture(TaskCanceledException cancelledTask) - ctxt.CallExceptionContinuation edi + ctxt.OnCancellation () ) |> unfake task @@ -1015,13 +1011,9 @@ namespace Microsoft.FSharp.Control else ctxt.cont ()) |> unfake - let cancelContinuation (cancelledTask: Task) : unit = + let cancelContinuation (_: Task) : unit = ctxt.trampolineHolder.ExecuteWithTrampoline (fun () -> - if useCcontForTaskCancellation then - ctxt.OnCancellation () - else - let edi = ExceptionDispatchInfo.Capture(new TaskCanceledException(cancelledTask)) - ctxt.CallExceptionContinuation edi + ctxt.OnCancellation () ) |> unfake task From 84dbe5415344f85ca9bba76608b198ad5d9d8116 Mon Sep 17 00:00:00 2001 From: Fraser Waters Date: Sat, 17 Aug 2019 17:05:13 +0100 Subject: [PATCH 3/3] Test that canceled AwaitTask doesn't raise TaskCanceledException --- .../Microsoft.FSharp.Control/AsyncType.fs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) 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 5e763ed1134..08a6173f874 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 @@ -402,7 +402,7 @@ type AsyncType() = Async.RunSynchronously(a, 1000) |> Assert.IsTrue [] - member this.AwaitTaskCancellation () = + member this.AwaitTaskTaskCancellation () = let test() = async { let tcs = new System.Threading.Tasks.TaskCompletionSource() tcs.SetCanceled() @@ -412,8 +412,22 @@ type AsyncType() = with :? System.OperationCanceledException -> return true } - Async.RunSynchronously(test()) |> Assert.IsTrue - + Async.RunSynchronously(test()) |> Assert.IsTrue + + [] + member this.AwaitTaskAsyncCancellation () = + let tcs = new System.Threading.Tasks.TaskCompletionSource() + let test = Async.AwaitTask tcs.Task + + use cts = new CancellationTokenSource() + cts.CancelAfter(250) + try + Async.RunSynchronously(test, cancellationToken=cts.Token) |> ignore + Assert.Fail("Expected async to throw") + with + | :? TaskCanceledException -> Assert.Fail("Did not expect TaskCanceledException") + | :? System.OperationCanceledException -> () + [] member this.AwaitTaskCancellationUntyped () = let test() = async {