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.

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

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

Open
5 tasks done
codingedgar opened this issue Feb 22, 2020 · 12 comments
Open
5 tasks done

Comments

@codingedgar
Copy link

codingedgar commented Feb 22, 2020

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:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • 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:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this
@FraserWaters-GR
Copy link

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: dotnet/fsharp#7357

@matthid
Copy link

matthid commented Feb 24, 2020

This is related to dotnet/fsharp#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)?

@codingedgar
Copy link
Author

@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?

@matthid
Copy link

matthid commented Feb 24, 2020

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

@dsyme
Copy link
Collaborator

dsyme commented Jun 15, 2022

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?

@abelbraaksma
Copy link
Member

abelbraaksma commented Dec 14, 2022

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

@dsyme
Copy link
Collaborator

dsyme commented Jan 9, 2023

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

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

I'll adjust the name of this suggestion

@dsyme dsyme changed the title AwaitTask without throwing AggregateException Async.Await overload (esp. AwaitTask without throwing AggregateException) Jan 9, 2023
@abelbraaksma
Copy link
Member

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.

@T-Gro
Copy link

T-Gro commented Jan 13, 2023

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

Async.Await(ValueTask)
ValueTask.toAsync

@TheAngryByrd
Copy link

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

@gusty
Copy link

gusty commented Feb 9, 2024

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.

@bartelink
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants