MailboxProcessor.PostAnd(Async)Reply never returns

[stmax82]

Below are 9 unit tests, 2 green / 7 red.

Situation

The red tests show cases where the MailboxProcessor.PostAnd(Async)Reply calls get stuck for eternity due to different reasons. I think things should never get stuck forever.

This might be related to case #4448, which was fixed a while ago, but there seem to be more cases with similar effects.

Workaround

Minimally set a cancellation token, e.g. cancellationToken=CancellationToken.None on the mailbox processor (which is intended only to govern the asynchronous computation running in the mailbox, and not the request-reply to fetch information). This changes the behaviour of MailboxProcessor.PostAndAsyncReply and friends to respect cancellation

    let mbox = MailboxProcessor.Start((fun inbox -> Async.Sleep(1000000)), cancellationToken=CancellationToken.None)

Repro

Tested with FSharp.Core 4.6.2.

namespace UnitTestProject1

open System
open System.Threading

open Microsoft.VisualStudio.TestTools.UnitTesting

[<TestClass>]
type AsyncCancellationTests () =
    // This test is GREEN because both MailboxProcess.Start and PostAndAsyncReply have a cancellation token
    // As expected - it throws an OperationCanceledException
    [<TestMethod; Timeout(2000)>]
    member this.Test1() =
        use cancel = new CancellationTokenSource(1000)

        let mbox = MailboxProcessor.Start((fun inbox -> Async.Sleep(1000000)), cancellationToken=cancel.Token)

        try
            Async.RunSynchronously(async {
                let! reply = mbox.PostAndAsyncReply(id)
                ()
            }, cancellationToken=cancel.Token)
        with
        | :? OperationCanceledException -> ()

    // This test is RED because PostAndAsyncReply doesn't get cancelled even though its containing async gets cancelled
    // Expected: it should throw an OperationCanceledException
    [<TestMethod; Timeout(2000)>]
    member this.Test2() =
        use cancel = new CancellationTokenSource(1000)

        let mbox = MailboxProcessor.Start((fun inbox -> Async.Sleep(1000000)))

        try
            Async.RunSynchronously(async {
                let! reply = mbox.PostAndAsyncReply(id)
                ()
            }, cancellationToken=cancel.Token)
        with
        | :? OperationCanceledException -> ()

    // This test is fishy GREEN even though its equal to Test2 (RED), except that the MailboxProcessor now gets a CancellationToken.None
    // As expected - it throws an OperationCanceledException
    [<TestMethod; Timeout(2000)>]
    member this.Test2_2() =
        use cancel = new CancellationTokenSource(1000)

        let mbox = MailboxProcessor.Start((fun inbox -> Async.Sleep(1000000)), cancellationToken=CancellationToken.None)

        try
            Async.RunSynchronously(async {
                let! reply = mbox.PostAndAsyncReply(id)
                ()
            }, cancellationToken=cancel.Token)
        with
        | :? OperationCanceledException -> ()

    // This test is RED because PostAndAsyncReply doesn't get cancelled when its MailboxProcessor gets cancelled
    // Expected: it should throw an OperationCanceledException
    [<TestMethod; Timeout(2000)>]
    member this.Test3() =
        use cancel = new CancellationTokenSource(1000)

        let mbox = MailboxProcessor.Start((fun inbox -> Async.Sleep(1000000)), cancellationToken=cancel.Token)

        try
            Async.RunSynchronously(async {
                let! reply = mbox.PostAndAsyncReply(id)
                ()
            })
        with
        | :? OperationCanceledException -> ()

    // This test is RED because PostAndAsyncReply gets stuck on a MailboxProcessor after it has been cancelled
    // Expected: it should throw an OperationCanceledException
    [<TestMethod; Timeout(2000)>]
    member this.Test4() =
        use cancel = new CancellationTokenSource()

        let mbox = MailboxProcessor.Start((fun inbox -> Async.Sleep(1000000)), cancellationToken=cancel.Token)

        cancel.Cancel()
        Thread.Sleep(1000)

        try
            Async.RunSynchronously(async {
                let! reply = mbox.PostAndAsyncReply(id)
                ()
            })
        with
        | :? OperationCanceledException -> ()

    // This test is RED because PostAndAsyncReply gets stuck on a disposed MailboxProcessor.
    // Expected: it should throw an ObjectDisposedException
    [<TestMethod; Timeout(2000)>]
    member this.Test5() =
        let mbox = MailboxProcessor.Start((fun inbox -> Async.Sleep(1000000)))

        (mbox :> IDisposable).Dispose()

        try
            Async.RunSynchronously(async {
                let! reply = mbox.PostAndAsyncReply(id)
                ()
            })
        with
        | :? ObjectDisposedException -> ()

    // This test is RED because PostAndReply gets stuck on a disposed MailboxProcessor.
    // Expected: it should throw an ObjectDisposedException
    [<TestMethod; Timeout(2000)>]
    member this.Test6() =
        let mbox = MailboxProcessor.Start((fun inbox -> Async.Sleep(1000000)))

        (mbox :> IDisposable).Dispose()

        try
            let reply = mbox.PostAndReply(id)
            ()
        with
        | :? ObjectDisposedException -> ()

    // This test is RED because PostAndReply gets stuck on a MailboxProcessor that has already exited.
    // Expected: it should throw some exception.. maybe ObjectDisposed or InvalidOperation?
    [<TestMethod; Timeout(2000)>]
    member this.Test7() =
        let mbox = MailboxProcessor.Start((fun inbox -> async { () }))

        Thread.Sleep(1000)

        try
            let reply = mbox.PostAndReply(id)
            ()
        with
        | :? ObjectDisposedException -> ()

    // This test is RED because PostAndAsyncReply gets stuck on a MailboxProcessor that has already exited.
    // Expected: it should throw some exception.. maybe ObjectDisposed or InvalidOperation?
    [<TestMethod; Timeout(2000)>]
    member this.Test8() =
        let mbox = MailboxProcessor.Start((fun inbox -> async { () }))

        Thread.Sleep(1000)

        try
            Async.RunSynchronously(async {
                let! reply = mbox.PostAndAsyncReply(id)
                ()
            })
        with
        | :? ObjectDisposedException -> ()

[dsyme]

Looking at these

Test2 - agreed this is a bug.

Tests 3, 4, 5, 6, 7, 8 - this is an open question - if the async for the mailbox exits, then should outstanding un-replied requests be cancelled, or fail, or simply never answer? Currently they simply never answer, but I can see the logic in throwing Disposed or Cancelled

Thanks for the excellent repro

[stmax82]

I guess the easiest way to handle these cases would be to throw ObjectDisposed for all un-replied (and newly incoming) requests after the async of the mailbox has exited - and no matter how it has exited (normally, exception, cancelled, disposed). So if it's not running anymore, you get ObjectDisposed.. easy to handle.

(I doubt it would help the caller if he had to handle 3 or 4 different exceptions depending on how the async of the mailbox has exited?)

Thanks for looking into these cases!