swlaschin / DomainModelingMadeFunctional

Extended code samples related to the book "Domain Modeling Made Functional". Buy the book here: https://pragprog.com/book/swdddf/domain-modeling-made-functional or here https://fsharpforfunandprofit.com/books/
Apache License 2.0
438 stars 84 forks source link

Use binding in an asyncResult computation expression #2

Closed kerams closed 3 years ago

kerams commented 6 years ago

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 6 years ago

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.

kerams commented 6 years ago

Think it makes sense, thanks.

kerams commented 6 years ago

@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 6 years ago

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

kerams commented 6 years ago

Alright, thanks a lot!

allentiak commented 3 years ago

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

swlaschin commented 3 years ago

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