swlaschin/DomainModelingMadeFunctional

Use binding in an asyncResult computation expression

kerams opened this issue · 7 comments

First off, thanks for the great book and this repo!

I think I'm running into a bug where use-bound values in asyncResult computation expressions are disposed prematurely. Consider this snippet:

let a = async {
    use f = { new IDisposable with member x.Dispose() = printfn "Disposed" }

    do! Async.Sleep 50
    printfn "async with do! ending"
    return Ok () } |> Async.RunSynchronously

printfn ""
let b = asyncResult {
    use f = { new IDisposable with member x.Dispose() = printfn "Disposed" }

    printfn "asyncResult without do! ending" } |> Async.RunSynchronously

printfn ""
let c = asyncResult {
    use f = { new IDisposable with member x.Dispose() = printfn "Disposed" }

    do! AsyncResult.sleep 10
    printfn "asyncResult with do! ending" } |> Async.RunSynchronously

Running this prints:

async with do! ending
Disposed

asyncResult without do! ending
Disposed

Disposed
asyncResult with do! ending

Do you have idea why f in the last case gets disposed early? Cheers.

mexx commented

Looks like the implementation of the TryWith, TryFinally and other build upon those functions are flawed.

If you look at the implementation of TryFinally (code):

member this.TryFinally(body, compensation) =
    try this.ReturnFrom(body())
    finally compensation() 

Lets replace the call to ReturnFrom with it's body, and add a type on the body parameters

member this.TryFinally(body: unit -> AsyncResult<_>, compensation) =
    try body()
    finally compensation() 

Can you spot the problem?

The body function is returning an Async, it might not have completed yet. However the control flow in the TryFinally function already goes on, and as the execution of the body function already ended, the finally block is executed with the compensation function.

If you compare the internals of this Builder with the internals of the AsyncBuilder from the FSharp.Core you can see, that there the try-finally and try-with constructs are lifted into the Async world. In this version of the builder the constructs are left outside, that's why you have the difference in the behaviour.

Think it makes sense, thanks.

@mexx I changed the 2 methods like this:

member this.TryWith(body, handler) = async {
    try
        let! b = body()
        return this.ReturnFrom(b)
    with e ->
        return handler e }

member this.TryFinally(body, compensation) = async {
    try
        let! b = body()
        return this.ReturnFrom(b)
    finally
        compensation() }

The dispose works as expected now, but I'm not sure if it's really as simple as this, because the AsyncBuilder implementation of these is a lot more involved (perhaps because of doing optimizations?).

mexx commented

@kerams I'm not certain that the calls to this.ReturnFrom are even necessary.

It's that simple, the AsyncBuilder implementation can't use any syntax sugar, like you do now with the async.

Alright, thanks a lot!

Just curious... Is this one fixed by #10?

(Finally revisiting all these issues!)
I think this is fixed now. Closing