Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't hide exceptions on the async cancellation-path #3257

Closed
wants to merge 9 commits into from
Closed

Don't hide exceptions on the async cancellation-path #3257

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7075338
don't hide potentially important exceptions when cancelling the workf…
matthid Jun 24, 2017
e742506
add more tests to check if StartAsTask and RunSynchronously behave th…
matthid Jun 24, 2017
b8a325e
use AsyncTaskMethodBuilder to fix the new tests
matthid Jun 24, 2017
304e5dc
more tests, flatten exceptions and improve information flow on AwaitT…
matthid Jun 25, 2017
a7d97e3
cleanup code
matthid Jun 25, 2017
d5881c9
Switch branches.
matthid Feb 9, 2018
ac0a9c1
some more fixes and PR hints.
matthid Feb 18, 2018
49c26bf
remove FX_NO_ASYNCTASKMETHODBUILDER
matthid Feb 22, 2018
bd08e6e
Merge branch 'master' into fix_3219_exception
matthid Mar 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 73 additions & 11 deletions src/fsharp/FSharp.Core/control.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,25 @@ 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) =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better name might be augmentOperationCancelledException. In this file protect normally means protect a function with a try/with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes makes sense

// 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|]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we throwing an AggregateException as the inner exception when there's only one exception? Feels a bit superfluous.

Copy link
Contributor Author

@matthid matthid Jun 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saul It is one of the details I have not completely figured out yet. On one hand you are correct on the other hand it feels wrong that you need to change your exception handling code when another exception is added...

Also I feel like people should not write too much code depending on those inner exceptions. So making things awkward is probably a good thing. Maybe we should add our own now exception type, because then we can add some additional warnings/documentation on its members...

At least I now know why @eiriktsarpalis is so skeptical about this. Even the bcl guys saw that this is hard to do afterwards and maintain compat.

But I still feel like hiding exceptions is not an option (again, bcl guys felt the same and added it in the c# await feature)

| :? AggregateException as a -> Array.append (a.Flatten().InnerExceptions |> Seq.toArray) [|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)
// 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
Expand All @@ -546,8 +565,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
Expand All @@ -566,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 (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() =
Expand Down Expand Up @@ -896,7 +915,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this token.IsCancellationRequested check so late. There is always a race here - we've reached the end of the computation and found an Error result. Why another check for cancellation so late?

I mean, it's certainly valid to check for cancellation so late, but is it necessary for the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is to have a consistent result. If I remember currently we can have different results depending of whether our Async contained a task or not. Or even worse how the code is formatted.
For example adding a try - with could lead to a different exception (even if the with block has nothing to do with the actual exception thrown). I think one of the tests actually checks this.

So basically this is to give a more consistent result when there is an interop with tasks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't yet see how this achieves consistency. IsCancellationRequested could become true the moment after we check it. And I don't think there are cases where we know a-priori that it was true coming into the exception continuation (we should never be taking the exception continuation once the cancellation continuation is taken)

I do understand that there is a case related to tasks where you want to make things consistent

For example adding a try - with could lead to a different exception (even if the with block has nothing to do with the actual exception thrown).

Could you give me an example?

Copy link
Contributor Author

@matthid matthid Feb 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the following test will fail:

    [<Test>]
    member this.StartAsTaskCancellationViaException () =
        let cts = new CancellationTokenSource()
        let tcs = TaskCompletionSource<unit>()
        let a = async {
            cts.CancelAfter (100)
            do! tcs.Task |> Async.AwaitTask }
        use t : Task<unit> =
            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, yet"

        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")

Basically without checking the cancellation-token check above we will yield no TaskCanceledException but an Exception. This is inconsistent because if you change the async to

        let a = async {
            try
                cts.CancelAfter (100)
                do! tcs.Task |> Async.AwaitTask
            with _ when false -> printfn "test" }

You will in-fact get the TaskCanceledException as you would expect. This is surprising behavior at the very least (ie we get a different exception only by adding a try ... with _ when false while everything else stays the same).

Therefore I added the additional checks on all the constructs. Is there a better option?

else Error(edi)
resultCell.RegisterResult(result,reuseThread=true))
(fun exn -> resultCell.RegisterResult(Canceled(exn),reuseThread=true))
computation
|> unfake
Expand Down Expand Up @@ -927,7 +950,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above. We have to do this for every relevant element

else Error(edi)
resultCell.RegisterResult(result,reuseThread=true))
(fun exn -> resultCell.RegisterResult(Canceled(exn),reuseThread=true))
computation)
|> unfake
Expand Down Expand Up @@ -969,9 +996,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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean: Does this define still exist?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one doesn't exist any more

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>()
#endif

// The contract:
// a) cancellation signal should always propagate to the computation
Expand All @@ -980,8 +1013,19 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and again.

protectExn (new TaskCanceledException()) edi :> exn
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
Expand Down Expand Up @@ -1178,14 +1222,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why allocate the fallback here when it isn't needed on all paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I guess I make this a function instead.

// 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's happened to the args.aux.token argument? Previously we were passing it in but now getCancelledException is being used and AFAICS it isn't setting the token

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is one of the changes where we changed to an TaskCanceledException exception if possible (or leave an existing exception in place). The TaskCanceledException improves interop when used from C# (ie when this is Started as Task on an higher level) but

  • The TaskCanceledException has no parameter to set a token, we associate with the Task instead
  • If we re-use an OperationCanceledException we assume it is correct
  • The third case shouldn't actually happen, but we could use an OperationCanceledException with the token there I guess (as long as we set the InnerException it should be fine). I'd prefer the TaskCanceledException as that's what actually happend (completedTask.IsCancelled is true)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll play around with changing this and there is probably one of the new tests that will fail. And I'll try to give some examples.
Because this might as well be a merge artifact (maybe it has been changed in master in the mean time)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok currently this doesn't have any observable impact, because useCcontForTaskCancellation is only true when FSCORE_PORTABLE_NEW is defined and only for the Sleep implementation (so this change would have changed the observable exception a bit in this scenario).
I changed this part by renaming the useCcontForTaskCancellation to useSimpleCcontForTaskCancellationAndLooseException to make it more clear that setting this to true has quite an impact. I also changed the code that useSimpleCcontForTaskCancellationAndLooseException has the old behavior (which I think is fine because Task.Sleep will probably never throw user-exceptions).

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
Expand All @@ -1195,12 +1254,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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here

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
Expand Down
114 changes: 110 additions & 4 deletions tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.")


Expand Down Expand Up @@ -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")

[<Test>]
member this.StartAsTaskCancellationViaException () =
let cts = new CancellationTokenSource()
let tcs = TaskCompletionSource<unit>()
let a = async {
cts.CancelAfter (100)
do! tcs.Task |> Async.AwaitTask }
#if FSCORE_PORTABLE_NEW || coreclr
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still exist?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @KevinRansom

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm taking a look through these various old flags now, will send a PR to flush this

let t : Task<unit> =
#else
use t : Task<unit> =
#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")

[<Test>]
member this.RunSynchronouslyCancellation () =
let cts = new CancellationTokenSource()
let tcs = TaskCompletionSource<unit>()
let a = async {
cts.CancelAfter (100)
do! tcs.Task |> Async.AwaitTask }
#if FSCORE_PORTABLE_NEW || coreclr
let t : Task<unit> =
#else
use t : Task<unit> =
#endif
Task.Run(new Func<unit>(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")

[<Test>]
member this.RunSynchronouslyCancellationViaException () =
let cts = new CancellationTokenSource()
let tcs = TaskCompletionSource<unit>()
let a = async {
cts.CancelAfter (100)
do! tcs.Task |> Async.AwaitTask }
#if FSCORE_PORTABLE_NEW || coreclr
let t : Task<unit> =
#else
use t : Task<unit> =
#endif
Task.Run(new Func<unit>(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")

[<Test>]
member this.StartTask () =
Expand Down
Loading