fslang-suggestions icon indicating copy to clipboard operation
fslang-suggestions copied to clipboard

Async.Await overload (esp. AwaitTask without throwing AggregateException)

Open codingedgar opened this issue 6 years ago • 12 comments

I propose including AwaitTaskCorrect (maybe with another name)

open System
open System.Threading.Tasks

type Async with
    static member AwaitTaskCorrect(task : Task) : Async<unit> =
        Async.FromContinuations(fun (sc,ec,cc) ->
            task.ContinueWith(fun (task:Task) ->
                if task.IsFaulted then
                    let e = task.Exception
                    if e.InnerExceptions.Count = 1 then ec e.InnerExceptions.[0]
                    else ec e
                elif task.IsCanceled then
                    ec(TaskCanceledException())
                else
                    sc ())
            |> ignore)

    static member AwaitTaskCorrect(task : Task<'T>) : Async<'T> =
        Async.FromContinuations(fun (sc,ec,cc) ->
            task.ContinueWith(fun (task:Task<'T>) ->
                if task.IsFaulted then
                    let e = task.Exception
                    if e.InnerExceptions.Count = 1 then ec e.InnerExceptions.[0]
                    else ec e
                elif task.IsCanceled then
                    ec(TaskCanceledException())
                else
                    sc task.Result)
            |> ignore)


// examples

let mkFailingTask exn = Task.Factory.StartNew<_>(fun () -> raise exn)

let test1 taskAwaiter =
    async {
        try
            return! taskAwaiter (mkFailingTask (ArgumentException()))
        with
        | :? ArgumentException -> return true
        | :? AggregateException -> return false
    } |> Async.RunSynchronously


test1 Async.AwaitTask // false
test1 Async.AwaitTaskCorrect // true


let test2 taskAwaiter =
    async {
        try
            return! taskAwaiter (mkFailingTask (AggregateException("kaboom!")))
        with
        | :? AggregateException as e when e.Message = "kaboom!" -> return true
        | :? AggregateException -> return false
    } |> Async.RunSynchronously

test2 Async.AwaitTask // false
test2 Async.AwaitTaskCorrect // true

The existing way of approaching this problem in F# is copying the snippet in your project, ref : jet/equinox#198

Pros and Cons

The advantages of making this adjustment to F#:

  • Provides a better implementation of Async.AwaitTask where the actual exception of a failing task is passed to the async mechanism.
  • Works as C# would handle taks

The disadvantages of making this adjustment to F# are:

  • Might be confusing having 2 AwaitTask
  • If AwaitTask is "fixed" then is a breaking change

Extra information

Estimated cost (XS, S, M, L, XL, XXL): No idea

Related suggestions: at this moment I don't know of any other related suggestions might edit in the future

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • [X] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • [X] I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • [X] This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • [X] This is not a breaking change to the F# language design
  • [X] I or my company would be willing to help implement and/or test this

codingedgar avatar Feb 22 '20 17:02 codingedgar

If breaking AwaitTask it would be good to fix up another of it's oddities at the same time, that it can't be cancelled: https://github.com/dotnet/fsharp/pull/7357

FraserWaters-GR avatar Feb 24 '20 09:02 FraserWaters-GR

This is related to https://github.com/dotnet/fsharp/pull/3257 Indeed, I discussed this quirk there as well. I'm skeptical about changing the exception type here... I'm also a bit skeptical about calling it Correct Maybe we can add an Overload and a Method parameter (unwrapException)?

matthid avatar Feb 24 '20 10:02 matthid

@matthid I’ll agree that the “Correct” part has to go, I was just honoring the Original Poster of the snippet.

The overload with a param that triggers the new behavior seems fine to me.

I checked that PR real quick and I’m not sure if it is exactly this, but really related should have been merged 😱

How do you deal with this? Is there any library that compensates this design right now?

codingedgar avatar Feb 24 '20 19:02 codingedgar

How do you deal with this? Is there any library that compensates this design right now?

I usually have some extension-methods to check for inner exceptions and/or unwrap the exception if needed

matthid avatar Feb 24 '20 19:02 matthid

I agree this should be solved. We need a better name - adding an optional argument seems reasonable though is a binary breaking change (though I believe it can be arranged that it's not a source breaking change on re-compilation).

I will mark this as approved in principle - would be great to have it as a community-led contribution

For the IsFaulted path should the code check that AggregateException is being thrown?

dsyme avatar Jun 15 '22 10:06 dsyme

Related, and a potential alternative path (that is: a community led TaskEx lib): https://github.com/fsprojects/FSharp.Control.TaskSeq/issues/128.

abelbraaksma avatar Dec 14 '22 01:12 abelbraaksma

We need a better name. Perhaps if we had an overloaded Async.Await(task) overloaded with

  • Async.Await(Async<T>)
  • Async.Await(Task<T>) -- adjusted Async.AwaitTaskCorrect semantics
  • Async.Await(ValueTask<T>)
  • Async.Await(task-like)

I'll adjust the name of this suggestion

dsyme avatar Jan 09 '23 17:01 dsyme

Note that Async.Await(ValueTask) will be NS2.1 only.

Is it wise to put all these overloads in Async? It may be hard to discover, people would probably expect task-like operations to be in Task.xxx, and same for ValueTask.

abelbraaksma avatar Jan 12 '23 02:01 abelbraaksma

We could add them symmetrically, similarly to how it is done for collection conversions

Async.Await(ValueTask) ValueTask.toAsync

T-Gro avatar Jan 13 '23 11:01 T-Gro

I decided to add them in IcedTasks under AsyncEx if anyone wants to give it a try.

TheAngryByrd avatar Jul 04 '23 20:07 TheAngryByrd

What about other Async functions doing surprising wrapping of aggregate exceptions?

For instance, I think we need something like:

type Async with
    static member StartImmediateAsTaskCorrect (computation: Async<'T>, ?cancellationToken) : Task<'T> =
        let cancellationToken = defaultArg cancellationToken Async.DefaultCancellationToken
        let ts = TaskCompletionSource<'T> ()
        Async.StartWithContinuations (
            computation,
            ts.SetResult,
            (function
                | :? AggregateException as agg -> ts.SetException agg.InnerExceptions
                | exn -> ts.SetException exn),
            (fun _ -> ts.SetCanceled ()),
            cancellationToken)
        ts.Task

Maybe I should create a separate issue.

gusty avatar Feb 09 '24 08:02 gusty

Think that's best covered under a separate issue (and revisiting Async.DefaultCancellationToken stuff can be talked about there too...)

bartelink avatar Feb 13 '24 09:02 bartelink