Binding to `Async<'T>` in a `task` CE uses StartAsTask, which involves a context switch

[abelbraaksma]

This was pointed out to me by @bartelink while working on TaskSeq, separately reported in this issue: fsprojects/FSharp.Control.TaskSeq#135, where I took the same approach as task.

The implementation for binding to an Async<'T> in the task computation expression builder is as follows:

member inline this.Bind(computation: Async<'TResult1>, continuation: ('TResult1 -> TaskCode<'TOverall, 'TResult2>)) : TaskCode<'TOverall, 'TResult2> =
    this.Bind(Async.StartAsTask computation, continuation)

Per the discussion here, its points by @gusty, and specifically this answer (#11043 (comment)) by @dsyme, shows that this is likely not how it should behave. I quote:

no implementation of let! ... and! ... should introduce calls to Async.StartChild, Async.Start or Async.Parallel - all of which start queue work in the thread pool. These calls must always be explicit. To be honest I feel it would be better if all of these had names like Async.StartChildInThreadPool, Async.StartInThreadPool and Async.ParallelInThreadPool. ANy introduction of the thread pool should be explicit

If it doesn't apply to let! ... and!..., it certainly shouldn't apply to let! in isolation. The method Async.StartAsTask forces a context switch and by above's analogy is essentially Async.StartInThreadpoolAsTask.

To bring Don's (and @gusty's in that thread) point home: we should be explicit and opt-in to parallelism or context switches. Here it's the opposite, we have to explicitly opt-out.

While this doesn't introduce parallelism, it may have subtle behavior related to side effects or updating mutables and the like.

TLDR: we should switch to Async.StartImmediateAsTask (if, hopefully, this doesn't introduce a backward compat issue we cannot come back from).

Repro steps

let currentBehavior() =
    let t = task {
        let a = Thread.CurrentThread.ManagedThreadId
        let! b = async {
            return Thread.CurrentThread.ManagedThreadId
        }
        let c = Thread.CurrentThread.ManagedThreadId
        return $"Before: {a}, in async: {b}, after async: {c}"
    }
    let d = Thread.CurrentThread.ManagedThreadId
    $"{t.Result}, after task: {d}"
    
let expectedBehavior() =
    let t = task {
        let a = Thread.CurrentThread.ManagedThreadId
        let! b =
            async {
                return Thread.CurrentThread.ManagedThreadId
            }
            |> Async.StartImmediateAsTask

        let c = Thread.CurrentThread.ManagedThreadId
        return $"Before: {a}, in async: {b}, after async: {c}"
    }
    let d = Thread.CurrentThread.ManagedThreadId
    $"{t.Result}, after task: {d}"

Expected behavior

No thread switch takes place. It should print "Before: 1, in async: 1, after: 1, after task: 1" in both cases.

Actual behavior

Two (!) extra thread switches take place. It actually prints this:

> currentBehavior();;
val it: string = "Before: 1, in async: 3, after async: 3, after task: 1"  // not good

> expectedBehavior();;
val it: string = "Before: 1, in async: 1, after async: 1, after task: 1"  // good

Known workarounds

Explicitly use Async.StartImmedateAsTask.

PS: this also applies to backgroundTask, perhaps even more so, as that already involves a context switch, so there's even less reason to add another context switch on top of it.

[T-Gro]

(this would probably need a new major version of the library, as there might be users relying on the current behaviour)

[abelbraaksma]

as there might be users relying on the current behaviour

Yes, or people are running into thread starvation or other unclear perf issues because of the thread switching. Trying hard to come up with a scenario we'd be breaking by fixing this (one that isn't too contrived), or a scenario that actually warrants this code to be the way it currently is (there may have been some discussion to this change that I couldn't find).

I should add to this report that async CE binding expressions do not jump to another thread in the same scenario. As such, it really feels like a bug in task. See this:

let asyncBehavior() =
    let t = async {
        let a = Thread.CurrentThread.ManagedThreadId
        let! b = async {
            return Thread.CurrentThread.ManagedThreadId
        }
        let c = Thread.CurrentThread.ManagedThreadId
        return $"Before: {a}, in async: {b}, after async: {c}"
    }
    let res = t |> Async.StartImmediateAsTask
    let d = Thread.CurrentThread.ManagedThreadId
    $"{res.Result}, after task: {d}"

which gives:

> currentBehavior();;
val it: string = "Before: 10, in async: 10, after async: 10, after task: 1"

This shows two things, the second potentially being another bug:

• OK: no context switch binding to async
• NOK: StartImmediateAsTask switches to a new thread, even though the docs explicitly say it doesn't do that (this is likely "by design", but as shown in the original post above, this need not happen necessarily).

[T-Gro]

The scenario I would be looking at for validity (if it even exists) of is when starting on a main/gui thread, and it would be intentional for the child calculation to jump off it.

I do agree that such things should be done explicitly (that is what backgrounTask{} is for), and I am inclining towards treating this is as a bugfix now as well.

(especially due to the negative resource utilization the current behaviour produces, and the quite common goal to mix task and async when using libraries coming from "different worlds")