Skip to content

Await completed Tasks on the same thread. - #11142

Merged
cartermp merged 4 commits into
dotnet:mainfrom
teo-tsirpanis:fix-9685
Feb 25, 2021
Merged

Await completed Tasks on the same thread.#11142
cartermp merged 4 commits into
dotnet:mainfrom
teo-tsirpanis:fix-9685

Conversation

@teo-tsirpanis

@teo-tsirpanisteo-tsirpanis commented Feb 25, 2021

Copy link
Copy Markdown
Contributor

This PR fixes#9685 by both setting a TaskContinuationFlags.RunSynchronously when attaching a continuation on the Tasks and by avoiding setting the continuation altogether when the task is completed.

The issue's title's "never" is now changed to "only when there is no sufficient stack", in which case the trampoline will take over.

Finally, the PR uses Thread.IsThreadPoolThread in one occasion it should.

@teo-tsirpanis
Run the continations synchronously in AsyncPrimitives.taskContinueWith.
9becffa
Fixesdotnet#9685.
Besides passing ExecuteSynchronously, if the task is already completed, the continuation is directly invoked, bypassing Task.ContinueWith.
Such trick is also performed by Roslyn.
dsyme
dsyme reviewed Feb 25, 2021
View reviewed changes
Comment threadsrc/fsharp/FSharp.Core/async.fs Outdated
dsyme
dsyme approved these changes Feb 25, 2021
View reviewed changes

@dsymedsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change requestedremoving a comment

@cartermp
cartermp merged commit 0fd033a into dotnet:mainFeb 25, 2021
@teo-tsirpanis
teo-tsirpanis deleted the fix-9685 branch February 25, 2021 21:12
@SzerSzer mentioned this pull request Jul 8, 2021
Closed
@dsyme

dsyme commented Jul 8, 2021

Copy link
Copy Markdown
Contributor

@teo-tsirpanis This caused a regression, see #11797. The change here installs a new trampoline on each completed task continuation - no new trampoline should be installed when we detect synchronous completion.

I'll push a PR with the fix

This was referenced Jul 8, 2021
Merged
Merged
@FoggyFinderFoggyFinder mentioned this pull request Jul 21, 2021
Closed
@dsyme

dsyme commented Jul 21, 2021

Copy link
Copy Markdown
Contributor

It looks like this PR causes other problems beyond #11797, through the use of ExecuteSynchronously clag on this line: https://github.com/dotnet/fsharp/pull/11142/files#diff-7330c080710da91733bbc5e1b5bdae30f4adf528f7847efbac3d86a0d4132b1dR945

I will remove that, this flag is not necessary for the asynchronous case, since we are already checking for synchronous completion.

This was referenced Jan 24, 2022
Closed
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsymedsymedsyme approved these changes
+2 more reviewers
@KevinRansomKevinRansomKevinRansom approved these changes
@cartermpcartermpcartermp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Awaiting completed Task with Async never runs on only one thread

4 participants

@teo-tsirpanis@dsyme@KevinRansom@cartermp