-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[RuntimeAsync] Fix for configured ValueTask sources #120704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
using System.Runtime.Versioning; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using System.Threading.Tasks.Sources; | ||
|
||
namespace System.Runtime.CompilerServices | ||
{ | ||
|
@@ -491,6 +492,49 @@ public static void HandleSuspended<T, TOps>(T task) where T : Task, ITaskComplet | |
} | ||
else if (calledTask != null) | ||
{ | ||
if (calledTask is IValueTaskAsTask vtTask) | ||
{ | ||
if (!vtTask.IsConfigured) | ||
{ | ||
// ValueTaskSource can be configured to use scheduling context or to ignore it. | ||
// The awaiter must inform the source whether it wants to continue on a context, | ||
// but the source may decide to ignore the suggestion. Since the behavior of | ||
// the source takes precedence, we clear the context flags from the awaiting | ||
// continuation (so it will run transparently on what source decides) and tell | ||
// the source that awaiting frame prefers to continue on a context. | ||
// The reason why we do it here is because the continuation chain builds from the | ||
// innermost frame out and when the leaf thunk links the head continuation, | ||
// it does not know if the caller wants to continue in a context. Thus the thunk | ||
// creates an "unconfigured" IValueTaskAsTask and we configure it here. | ||
ValueTaskSourceOnCompletedFlags configFlags = ValueTaskSourceOnCompletedFlags.None; | ||
CorInfoContinuationFlags continuationFlags = headContinuation.Next!.Flags; | ||
|
||
const CorInfoContinuationFlags continueOnContextFlags = | ||
CorInfoContinuationFlags.CORINFO_CONTINUATION_CONTINUE_ON_CAPTURED_SYNCHRONIZATION_CONTEXT | | ||
CorInfoContinuationFlags.CORINFO_CONTINUATION_CONTINUE_ON_CAPTURED_TASK_SCHEDULER; | ||
|
||
if ((continuationFlags & continueOnContextFlags) != 0) | ||
{ | ||
// effectively move context flags from headContinuation to the source config. | ||
configFlags |= ValueTaskSourceOnCompletedFlags.UseSchedulingContext; | ||
headContinuation.Next!.Flags &= ~continueOnContextFlags; | ||
} | ||
|
||
vtTask.Configure(configFlags); | ||
|
||
if (!calledTask.TryAddCompletionAction(task)) | ||
{ | ||
// calledTask has already completed and we need to schedule | ||
// our code for execution ourselves. | ||
// Restore the continuation flags before doing that. | ||
headContinuation.Next!.Flags = continuationFlags; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is subtle. If the ValueTask has already completed, it missed its chance to decide on how continuations run. In such case it is up to the awaiter. There are tests sensitive to this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do not do The complications with using |
||
ThreadPool.UnsafeQueueUserWorkItemInternal(task, preferLocal: true); | ||
} | ||
|
||
return; | ||
} | ||
} | ||
|
||
// Runtime async callable wrapper for task returning | ||
// method. This implements the context transparent | ||
// forwarding and makes these wrappers minimal cost. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
|
||
<PropertyGroup> | ||
<RunAnalyzers>true</RunAnalyzers> | ||
<NoWarn>$(NoWarn);xUnit1013;CS1998</NoWarn> | ||
<NoWarn>$(NoWarn);xUnit1013;CS1998;SYSLIB5007</NoWarn> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: revert before merging this and the following changes that enable runtime async in the PR for testing purposes. |
||
<EnableNETAnalyzers>false</EnableNETAnalyzers> | ||
<Features>$(Features);runtime-async=on</Features> | ||
</PropertyGroup> | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have a more efficient solution that skips allocation of
ValueTaskSourceAsTask
entirely by stashing an unwrappedValueTaskSource
and callingValueTaskSourceAsTask
equivalent code from the continuation dispatcher.At this point we are looking for correctness, so I went with a smaller change. Optimizations can come later, if this scenario is common/interesting enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO if we are going to end up with another path here, then we should just make that change now and get the correctness as a side effect. It will also look more obviously correct since it will match exactly what
ValueTaskAwaiter.OnCompleted
does. That means less work for me trying to understand what the issue here was :-)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure optimizing this will make it easier to follow. The key of the change is unsetting the continuation flags of the awaiting continuation and passing equivalent flag to the
ValueTaskSourceAsTask
that we are waiting on. That part of the fix will need to stay even if we optimize away allocating aValueTaskSourceAsTask
.Skipping allocation of
ValueTaskSourceAsTask
would result incalledTask
becoming anobject
or adding yet another field in the awaitState.And for non-source ValueTask, we would still want
.AsTask
.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to agree that this is a correct fix before optimizing further.
I think it is a correct fix, but maybe I miss some nuance or perhaps there is a better way to achieve the same effect.
Like - the change defers configuring the source until we are in
HandleSuspended
. That bothers be a bit, but I think that is ok and there is no way around that. At the time ofAsTask
we do not know if caller/awaiter up the stack is configured or not.(NOTE: A source-wrapping
ValueTask
cannot come from an async method, so.AsTask
for it will be always called from a context-transparent thunk. It would be the one level up frame that did the actualawait
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand. Unlike
Task
there is no way to do a transparent await forValueTaskSource
since the configuration gets passed to 3rd party code. So we truly do need to get the configuration value from the caller.I'll look more deeply tomorrow at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be useful. I'll add such test.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only that. Also:
if the 3rd party wants to run continuations on a default context, then the awaiting continuation should run on that (even though itself has captured nondefault context).
There is no opposite case though. If the await had
ConfigureAwait(false)
the continuation runs on default context.That is, at least, expected in this test:
runtime/src/libraries/System.IO.Pipelines/tests/SchedulerFacts.cs
Line 136 in ce717ac
an opposite scenario is also possible, although uncommon (Pipelines do not do that).
That is when await says
false
but the source still runs continuations on captured context.An additional nuance - if the task has completed by the time we try to add a continuation, then it has no say in how continuation runs and continuation runs on what await has captured.
(it seems there are possibilities for races here, but it might not be a big deal in real scenarios)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runtime async infrastructures tries to implement the same semantics as
Task.UnsafeSetContinuationForAwait
does, in terms of resumption behavior. But these are not necessarily the behaviors matched by custom implementations ofIValueTaskSource
. They can be as broken as they like since everything is left up to user controlled code for them.I guess there is a question of how far we need to go with replicating asyncv1 behavior for "broken" implementations of
IValueTaskSource
that do not conform to the standard resumption behavior ofTask
. If we go for full compatibility it is not clear to me yet how this affects our ability to optimize calls toValueTask
returning methods, for example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not true. The opposite is also possible. That is when the source captures the scheduling context and posts to it even when await was configured to false.
It is just not common behavior to do so, but if source does that, it wins over what await wants.
It is also possible for the source to pick whatever random context and run continuations on that. I think we can ignore such possibility as a broken implementation. The only choice should be between the scheduling context and the default.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just need to match the part where if we see incomplete ValueTask that wraps a source we should let the source to run the continuation callback in whatever way it wants (basically ignore whether the await was configured), but we also need to tell the source what we had on the await as there is a way to tell and the source might consider that.
I see only two uses of this part of API in the Libraries:
Parallel.ForEach
overAsyncEnumerable
scenario.IO.Pipe
). In such case it picks the most relaxed way of invoking between its own config and what comes from the await config.I could imagine a case where source only considers its own config.
Supporting these three cases should be sufficiently compatible.