dotnet / dotnet/fsharp

TryFinallyAsync implementation ignores potential exceptions in TryFinally

Open
#16,189 1 comment 2 reactions 0 assignees View on GitHub
Area-Compiler-StateMachines Breaking-change Bug Impact-Medium
Dominant language
F#
Stars
4.3k
Forks
876
Avg merge
4d 22h
Merged PRs (30d)
144

Description

Currently implementation of TryFinallyAsync is defined as:
```fsharp
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):
```fsharp
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:
```fsharp
[]
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(fun () -> task {
use _ = disposable
()
})
()
}
```

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

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.