[RuntimeAsync] Fix for configured ValueTask sources #120704
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,50 @@ 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) | ||
| { | ||
| // if await has captured some context, inform the source | ||
| configFlags |= ValueTaskSourceOnCompletedFlags.UseSchedulingContext; | ||
| } | ||
|
|
||
| // clear continuation flags, so that continuation runs transparently | ||
| headContinuation.Next!.Flags &= ~continueFlags; | ||
|
There was a problem hiding this comment. This makes it illegal for the JIT to inline or tailcall I assume this is necessary for this particular case:
I wonder if we can go with a change in behavior that aligns things with how task-task resumptions work. There was a problem hiding this comment. @stephentoub I wonder what your opinion is here. We have some issues replicating exactly the behavior of The problem is that replicating this behavior requires keeping around information about which continuation now has the interface IFace { ValueTask VTMethod(); }
static async ValueTask M1(IFace i)
{
await M2(i);
Console.WriteLine(SynchronizationContext.Current); // has to be the same as before M2 call, since M2 is a regular ValueTask-ValueTask call
}
static async ValueTask M2(IFace i)
{
await i.VTMethod(); // can this be a tailcall? No, because i.VTMethod may be backed by an `IValueTaskSource` that executes the continuation somewhere else
}It is not a massive concession to disable tailcalls for There was a problem hiding this comment.
Just to be sure - these are not tests preserving some implementation quirks. There are features like Ex: PipeOptions.UseSynchronizationContext Property I think we will have to support these scenarios for compat reasons. There was a problem hiding this comment.
Yes, seems like it. Can you add the necessary code that disables tailcalls to There is also code in that function that optimizes out continuation context handling when Can you also add a test for the scenario? |
||
| vtTask.Configure(configFlags); | ||
|
|
||
| if (!calledTask.TryAddCompletionAction(task)) | ||
| { | ||
| // calledTask has already completed and we need to schedule | ||
| // the continuation for execution ourselves. | ||
| // Restore the continuation flags before doing that. | ||
| headContinuation.Next!.Flags = continuationFlags; | ||
|
There was a problem hiding this comment. 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. 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. | ||
|
|
||
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
ValueTaskSourceAsTaskentirely by stashing an unwrappedValueTaskSourceand callingValueTaskSourceAsTaskequivalent 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.OnCompleteddoes. 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
ValueTaskSourceAsTaskthat we are waiting on. That part of the fix will need to stay even if we optimize away allocating aValueTaskSourceAsTask.Skipping allocation of
ValueTaskSourceAsTaskwould result incalledTaskbecoming anobjector 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 ofAsTaskwe do not know if caller/awaiter up the stack is configured or not.(NOTE: A source-wrapping
ValueTaskcannot come from an async method, so.AsTaskfor 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
Taskthere is no way to do a transparent await forValueTaskSourcesince 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
falsebut 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.UnsafeSetContinuationForAwaitdoes, 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
IValueTaskSourcethat 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 toValueTaskreturning 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.ForEachoverAsyncEnumerablescenario.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.