Executing a list of asynchronous computations hangs after updating to FSharp.Core 5.0.2

[raquila]

I'm running into an issue where some pre-existing code is no longer functioning as expected after updating to any version of FSharp.Core >= 5.0.2. The project is referencing a f# library that uses async from a C# mvc web project running .net framework 4.8. I'm noticing that Async.RunSynchronously just hangs and never returns when called from the web project. I was able to create a simple test project to reproduce the issue. The visual studio solution contains 3 projects; a f# library, C# console, and C# MVC project. All projects are .net framework 4.8. The same test function below works from the C# console project but not the C# MVC web project. It does work in the web project using FSharp.Core <=5.0.1 but all versions after don't seem to work.

namespace ReproduceAsyncError

module TestWork =

    let test_parser x =
        match System.Decimal.TryParse(x) with
        | (true, d) -> Ok(d)
        | _ -> Error(sprintf "'%s' is not a number" x)

    let async_work x = async { return test_parser (x) }

    let task_work = fun x -> task { return test_parser (x) }

    let async_task_work x =
        async {
            let! t = task_work (x) |> Async.AwaitTask
            return t
        }

    //hangs
    let do_something_in_parallel () =
        let x =
            [ for i in 1 .. 50 -> i.ToString() ]
            @ [ "Not A Number" ]
              @ [ for i in 100 .. 150 -> i.ToString() ]

        let transactions =
            x
            |> List.map (fun x -> (async_work x))
            |> Async.Parallel
            |> Async.RunSynchronously

        transactions

    //hangs
    let do_something_in_parallel_with_async_tasks () =
        let x =
            [ for i in 1 .. 50 -> i.ToString() ]
            @ [ "Not A Number" ]
              @ [ for i in 100 .. 150 -> i.ToString() ]

        let transactions =
            x
            |> List.map (fun x -> (async_task_work x))
            |> Async.Parallel
            |> Async.RunSynchronously

        transactions

    //hangs
    let do_something_sequential () =
        let x =
            [ for i in 1 .. 50 -> i.ToString() ]
            @ [ "Not A Number" ]
              @ [ for i in 100 .. 150 -> i.ToString() ]

        let transactions =
            x
            |> List.map (fun x -> (async_work x))
            |> Async.Sequential
            |> Async.RunSynchronously

        transactions

    //working
    let do_single_async () =
        let transactions = async_work "1" |> Async.RunSynchronously
        transactions

    //working
    let do_something_in_parallel_with_tasks () =
        let x = [ for i in 1 .. 50 -> i.ToString() ]

        let transactions =
            x
            |> Array.ofList
            |> Array.Parallel.map (fun p -> task_work(p).Result)

        transactions

using System.Web.Mvc;

namespace TestWebApplication.Controllers
{
    public class HomeController : Controller
    {
        public ActionResult Index()
        {
            var p = ReproduceAsyncError.TestWork.do_something_in_parallel();
            //var p = ReproduceAsyncError.TestWork.do_single_async();
            //var p = ReproduceAsyncError.TestWork.do_something_in_parallel_with_async_tasks();
            //var p = ReproduceAsyncError.TestWork.do_something_sequential();
            //var p = ReproduceAsyncError.TestWork.do_something_in_parallel_with_tasks();
            return View();
        }

        public ActionResult About()
        {
            ViewBag.Message = "Your application description page.";

            return View();
        }

        public ActionResult Contact()
        {
            ViewBag.Message = "Your contact page.";

            return View();
        }
    }
}

ReproduceAsyncError.zip

[dsyme]

May be related to #11861 though looking through the repro it seems to be reported with just straight "async" too (no tasks or AwaitTask)

The repro looks a little odd because it uses "task" but "task" is only supported in F# 6 and FSharp.Core 6.0.0+. So the mention of 5.0.1 seems strange, but I presume that's referring to the task-free repro involving async only.

[raquila]

Correct, I added some other methods to see how they would behave in the latest version of Fsharp.Core. The current code that represents the problems of the existing code base we have that works prior to the 5.0.2 is below.

let do_something_in_parallel () =
        let x =
            [ for i in 1 .. 50 -> i.ToString() ]
            @ [ "Not A Number" ]
              @ [ for i in 100 .. 150 -> i.ToString() ]

        let transactions =
            x
            |> List.map (fun x -> (async_work x))
            |> Async.Parallel
            |> Async.RunSynchronously

        transactions

[dsyme]

The current code that represents the problems of the existing code base we have that works prior to the 5.0.2 is below.

The change that caused this bug is #11142. This change caused numerous other issues, as linked from that thread.

However, using RunSynchronously from SynchronizationContext thread (as used in ASP.NET) is really not recommended - it fundamentally runs the risk of blocking the processing on that thread, and that's precisely what must be happening here (we should double check).

I haven't confirmed this yet, but my understanding is that you should use SwitchToNewThread, e.g.

        let transactions =
            async { 
                do! Async.SwitchToNewThread()
                return! 
                    x
                    |> List.map (fun x -> (async_work x))
                    |> Async.Parallel
            } |> Async.RunSynchronously

[raquila]

Should we always be calling Async.SwitchToNewThread() just to be safe incase code like this ends up being called from a SynchronizationContext thread without realizing it? Are there any issues with always calling SwitchToNewThread?

[dsyme]

Should we always be calling Async.SwitchToNewThread() just to be safe incase code like this ends up being called from a SynchronizationContext thread without realizing it? Are there any issues with always calling SwitchToNewThread?

cc @teo-tsirpanis @cartermp

My analysis:

1. Prior to Await completed Tasks on the same thread. #11142 a "switch to thread pool" is done if SynchronizationContext is non-null (or a timeout is specified)

2. After Await completed Tasks on the same thread. #11142 a "switch to thread pool" is done if Thread.CurrentThread.IsThreadPoolThread is false (or a timeout is specified). In the context of your web app, I assume IsThreadPoolThread is true, and SynchronizationContext is non-null. So a switch is no longer being performed.

3. As a workaround, you should be able to use either SwitchToNewThread or SwitchToThreadPool as shown above. I don't see any specific problems with this, though you should test. Currently, the parallel computation will simply not be able to complete.

In retrospect, this part of #11142 should not have been accepted - there was no strong rationale for the change and it caused other problems. We have learned a lesson here and it's essential that we don't take PRs for this sort of thing without really proper analysis of the ramifications. I am responsible as one of the reviewers.

Going forward, it is reasonable to change the Async.RunSynchronusly to switch to the thread pool when either SynchronizationContext is non-null or Thread.CurrentThread.IsThreadPoolThread is false. However I'm somewhat cautious to make additional changes. I'll propose a PR and we can discuss.

[teo-tsirpanis]

The code's original intent had always been to check whether we are in a thread pool thread. I had switched the "is the SC null" approximated check to an precise Thread.CurrentThread.IsThreadPoolThread based on that comment and because by that time FSharp.Core abandoned .NET Standard 1.x which did not have this property. Considering that this #11142 keeps causing trouble and as its author, I feel responsible as well.

However, @raquila's code does the ever-cautioned error of blocking on asynchronous code inside a SynchronizationContext. Had they done the same thing on C# the system would hang and it would be their fault. Other than to maintain backwards compatibility and make this bad practice work, I am not sure whether we should introduce additional logic to handle it.

[dsyme]

The code's original intent had always been to check whether we are in a thread pool thread.

Yes, in retrospect this comment in the code was incorrect - and its presence is what confused me as well - I'm not really sure who authored that comment (it doesn't really matter) but it was vague rather than ground-truth-desired-spec. Anyway I think the spec of "check we are in a thread pool thread where SynchronizationContext.Current is null" is a good, strong, clear spec that covers both backwards compat and gives a strong-enough guarantee.

However, @raquila's code does the ever-cautioned error of blocking on asynchronous code inside a SynchronizationContext. Had they done the same thing on C# the system would hang and it would be their fault. Other than to maintain backwards compatibility and make this bad practice work, I am not sure whether we should introduce additional logic to handle it.

Yes, for backwards compat we should make this practice work. It worked like this since F# 2.0.

[raquila]

Just a little background, the code originally was written to run on the backend within a windows service context. The code still works when executed from that service with the 6.0.1 version of FSharp.Core. Over time it ended up also being referenced through a web frontend in order to reuse the functionality elsewhere. Since it worked, nobody was the wiser and there was no indication that this won't continue to work. SwitchToNewThread seems to fix the issue for now.

[dsyme]

Fixed by #12638