TryFinallyAsync implementation ignores potential exceptions in TryFinally

[En3Tho]

Currently implementation of TryFinallyAsync is defined as:

member inline internal this.TryFinallyAsync
        (
            body: TaskCode<'TOverall, 'T>,
            compensation: unit -> ValueTask
        ) : TaskCode<'TOverall, 'T> =
        ResumableCode.TryFinallyAsync(
            body,
            ResumableCode<_, _>(fun sm ->
                if __useResumableCode then
                    let mutable __stack_condition_fin = true
                    let __stack_vtask = compensation ()

                    if not __stack_vtask.IsCompleted then
                        let mutable awaiter = __stack_vtask.GetAwaiter()
                        let __stack_yield_fin = ResumableCode.Yield().Invoke(&sm)
                        __stack_condition_fin <- __stack_yield_fin

                        if not __stack_condition_fin then
                            sm.Data.MethodBuilder.AwaitUnsafeOnCompleted(&awaiter, &sm)

                    __stack_condition_fin
                else
                    let vtask = compensation ()
                    let mutable awaiter = vtask.GetAwaiter()

                    let cont =
                        TaskResumptionFunc<'TOverall>(fun sm ->
                            awaiter.GetResult() |> ignore
                            true)

                    // shortcut to continue immediately
                    if awaiter.IsCompleted then
                        true
                    else
                        sm.ResumptionDynamicInfo.ResumptionData <- (awaiter :> ICriticalNotifyCompletion)
                        sm.ResumptionDynamicInfo.ResumptionFunc <- cont
                        false)
        )

Note that it never calls awaiter.GetResult() meaning that if ValueTask was compelted synchronously it will throw but if not then it will just ignore result of the async computation.

Proposed fix is (matches .Bind implementation for example):

member inline internal this.TryFinallyAsync
        (
            body: TaskCode<'TOverall, 'T>,
            compensation: unit -> ValueTask
        ) : TaskCode<'TOverall, 'T> =
        ResumableCode.TryFinallyAsync(
            body,
            ResumableCode<_, _>(fun sm ->
                if __useResumableCode then
                    let mutable __stack_condition_fin = true
                    let __stack_vtask = compensation ()
                    let mutable awaiter = __stack_vtask.GetAwaiter()
                    
                    if not awaiter.IsCompleted then                        
                        let __stack_yield_fin = ResumableCode.Yield().Invoke(&sm)
                        __stack_condition_fin <- __stack_yield_fin

                    if __stack_condition_fin then
                        awaiter.GetResult()
                    else
                        sm.Data.MethodBuilder.AwaitUnsafeOnCompleted(&awaiter, &sm)

                    __stack_condition_fin
                else
                    let vtask = compensation ()
                    let mutable awaiter = vtask.GetAwaiter()

                    let cont =
                        TaskResumptionFunc<'TOverall>(fun sm ->
                            awaiter.GetResult() |> ignore
                            true)

                    // shortcut to continue immediately
                    if awaiter.IsCompleted then
                        cont.Invoke(&sm)
                    else
                        sm.ResumptionDynamicInfo.ResumptionData <- (awaiter :> ICriticalNotifyCompletion)
                        sm.ResumptionDynamicInfo.ResumptionFunc <- cont
                        false)
        )

Test code is:

[<Fact>]
let``task should propagate exception if thrown in DisposeAsync``() = task {
    let disposable = { new IAsyncDisposable with
        member _.DisposeAsync() = ValueTask(task {
            do! Task.Delay(1)
            failwith "boom"
        })
    }

    let! _ = Assert.ThrowsAsync<Exception>(fun () -> task {
        use _ = disposable
        ()
    })
    ()
}

Current behavior:

Assert.Throws() Failure
Expected: typeof(System.Exception)
Actual:   (No exception was thrown)

[abelbraaksma]

yeah, I think this is a bug. @abonie is right to mark this as a breaking change, unfortunately, because currently, finally will not throw in the scenario you give, meaning that there will be code in the wild that may rely on this behavior.

Granted: it should throw, for sure, if your code executes. However, be aware that it is considered bad practice to throw from a Dispose method, esp since in many coding patterns, Dispose can be called from the finalizer. And if a finalizer throws, it causes the whole process to fail fast. More info on dispose patterns in the topic of the Dispose method.