Repository navigation
Binding to Async<'T> in a task CE uses StartAsTask, which involves a context switch #14490
Description
Activity
(this would probably need a new major version of the library, as there might be users relying on the current behaviour)
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
asyncCE binding expressions do not jump to another thread in the same scenario. As such, it really feels like a bug intask. 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:
StartImmediateAsTaskswitches 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).
- OK: no context switch binding to
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")
- linked a pull request that will close this issueBind of Async<> within task{} should start on the same thread #14499
on Dec 20, 2022 I am now too inclining to treating it like a bugfix, even in the GUI thread situation.
- NOK:
StartImmediateAsTaskswitches 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).
IIRC
RunSynchronouslyhas a similar behavior - it's hopping to a thread pool thread iff not already on one (have not seen docs as to why that would be, but if it's a one-off)The async tutorial does not even hint at this behavior; perhaps it should? I'd be interested to know what the reasoning for this hop is (it's also probably academic given how risky changing it would be).
- NOK:
Yes, if possible we should fix this for the Async scenario as well, but considering it’s been around much longer may make this a less likely candidate to be allowed a fix in a minor release.
Async.RunSynchronously has analogies to the backgroundTask{} in that behaviour, and I believe it was a good intention to prevent hangs when using it in UI applications.
Due to the amount of code using it, I do not think we can turn the behavior around.
We could introduce a new variant of it that would not switch to a background thread, and name it differently.Reacted by Ruben Bartelink and Abel BraaksmaIf this (see https://stackoverflow.com/questions/54312750/does-async-runsynchronously-method-block) is true,
RunSynchronousis blocking, which means it (probably) doesn’t help with deadlocks.But I agree, there should be an overload with a different name.
- moved this from Not Planned to Done in F# Compiler and Tooling
on Dec 27, 2022 Isn't this what is wanted in the end? I must have overlooked this before.
fsharp/fslang-suggestions#1042It is already implemented in the compiler's internal test suite, could be moved to Fsharp.Core:
https://github.com/dotnet/fsharp/pull/11788/files#diff-0b39085aa15d27c80662a1386282d6c4a9d2e8747b3124024d755bb96f9094f6Reacted by Ruben Bartelink and Abel BraaksmaThat does seem to line everything up nicely - I guess using
Immediateas the term for running inline (vs theRunSynchronously/StartAsTasktransition to threadpool thread if not on one already semantic) gives a hook for the docs to cover it.@T-Gro yes, that's exactly what we'd need, I missed it too on earlier searches. Thanks!
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 astask.The implementation for binding to an
Async<'T>in thetaskcomputation expression builder is as follows: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:
If it doesn't apply to
let! ... and!..., it certainly shouldn't apply tolet!in isolation. The methodAsync.StartAsTaskforces a context switch and by above's analogy is essentiallyAsync.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
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:
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.