[RuntimeAsync] Fix for configured ValueTask sources

[VSadov]

Fixes: #120709

There is a scenario where ValueTask decides on whether to run continuations on the scheduling context or not.

It happens in a case when an await calls into a ValueTask that wraps a ValueTaskSource.
In such case we may have a nontrivial context provided to the ValueTaskSource at the time of the await, but ValueTaskSource may choose to ignore the context when running callbacks.

A typical test sensitive to this looks like:

runtime/src/libraries/System.IO.Pipelines/tests/SchedulerFacts.cs

Lines 48 to 89 in ce717ac

	public async Task UseSynchronizationContextFalseIgnoresSyncContextForReaderScheduler()
	{
	SynchronizationContext previous = SynchronizationContext.Current;
	var sc = new CustomSynchronizationContext();
	try
	{
	SynchronizationContext.SetSynchronizationContext(sc);
	var pipe = new Pipe(new PipeOptions(useSynchronizationContext: false));
	Func<Task> doRead = async () =>
	{
	ReadResult result = await pipe.Reader.ReadAsync();
	pipe.Reader.AdvanceTo(result.Buffer.End, result.Buffer.End);
	pipe.Reader.Complete();
	};
	// This needs to run on the current SynchronizationContext
	Task reading = doRead();
	PipeWriter buffer = pipe.Writer;
	buffer.Write("Hello World"u8.ToArray());
	// Don't run code on our sync context (we just want to make sure the callbacks)
	// are scheduled on the sync context
	await buffer.FlushAsync().ConfigureAwait(false);
	// Nothing posted to the sync context
	Assert.Equal(0, sc.Callbacks.Count);
	pipe.Writer.Complete();
	// Don't run code on our sync context
	await reading.ConfigureAwait(false);
	}
	finally
	{
	SynchronizationContext.SetSynchronizationContext(previous);
	}
	}

The part

                    ReadResult result = await pipe.Reader.ReadAsync();

should behave as

                    ReadResult result = await pipe.Reader.ReadAsync().ConfigureAwait(false);

Otherwise we end up posting the doRead continuation to the tracking CustomSynchronizationContext which only tracks Posts and does not run them, thus the test makes no progress.

[VSadov]

TODO: revert before merging this and the following changes that enable runtime async in the PR for testing purposes.

[VSadov]

CC: @stephentoub Are we getting the desired behavior correctly?

We do have testcases sensitive to these behaviors, which fail without this fix.

[VSadov]

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.

[VSadov]

We may have a more efficient solution that skips allocation of ValueTaskSourceAsTask entirely by stashing an unwrapped ValueTaskSource and calling ValueTaskSourceAsTask 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.

[jakobbotsch]

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 :-)

[VSadov]

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 a ValueTaskSourceAsTask.

Skipping allocation of ValueTaskSourceAsTask would result in calledTask becoming an object or adding yet another field in the awaitState.
And for non-source ValueTask, we would still want .AsTask.

[VSadov]

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 of AsTask 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 actual await)

[jakobbotsch]

I think I understand. Unlike Task there is no way to do a transparent await for ValueTaskSource 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.

[VSadov]

We might want a very low level compatibility test for this -- something that just verifies that a custom IValueTaskSource sees the expected flags passed to its OnCompleted.

Yes, it would be useful. I'll add such test.

[VSadov]

I think I understand. Unlike Task there is no way to do a transparent await for ValueTaskSource since the configuration gets passed to 3rd party code.

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

  public async Task DefaultReaderSchedulerIgnoresSyncContextIfConfigureAwaitFalse()

• 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)

[jakobbotsch]

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 of IValueTaskSource. 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 of Task. If we go for full compatibility it is not clear to me yet how this affects our ability to optimize calls to ValueTask returning methods, for example.

[VSadov]

There is no opposite case though.

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.

[VSadov]

They can be as broken as they like since everything is left up to user controlled code for them.

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:

• the source is used for other purposes than dealing with the context - i.e. for pooling and reusing awaitable resources. In such case the source cannot be configured and it just goes with what it gets passed based on the await config. This is a Parallel.ForEach over AsyncEnumerable scenario.
• the source is used specifically for the purposes of overriding callback calling strategy (like the one in 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.

[Copilot]

Pull Request Overview

This PR addresses a scenario where ValueTask doesn't properly handle configuration of async continuations when wrapping ValueTaskSource implementations. The fix ensures that unconfigured awaits behave as if they were configured with ConfigureAwait(false) when the underlying ValueTaskSource can choose to ignore scheduling context.

Key changes:

• Introduces deferred configuration mechanism for ValueTaskSource callbacks
• Updates runtime async thunks to use unconfigured task conversion
• Enables runtime async tests that were previously disabled

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ValueTask.cs	Adds AsUnconfiguredTask() methods and deferred configuration support for ValueTaskSource instances
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs	Implements configuration logic to handle context flags between continuations and ValueTaskSource
src/coreclr/vm/corelib.h	Updates method bindings to use unconfigured task conversion
src/coreclr/vm/asyncthunks.cpp	Changes async thunks to call AsUnconfiguredTask instead of AsTask
src/coreclr/inc/clrconfigvalues.h	Enables runtime async support by default
src/tests/async/Directory.Build.targets	Removes project build disable for runtime async tests
src/tests/async/Directory.Build.props	Adds warning suppression for new analyzer rule
src/tests/async/struct/struct.cs	Adds attributes to opt out of runtime async generation for struct methods