Skip to content

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Oct 3, 2025

Runtime async callable thunks were using TaskAwaiter directly, but that has the normal await semantics which will either post continuations to the captured synchronization context or to the thread pool. This introduces an observable behavior change with async1 where sometimes even configured awaits will end up posting back to a captured synchronization context.

For example, consider an example like:

private static async Task Foo()
{
    SynchronizationContext.SetSynchronizationContext(new TrackingSynchronizationContext());
    await Task.Delay(1000).ConfigureAwait(false);
}

Before this change the runtime async call to Task.Delay creates a runtime async callback thunk that roughly looks like

Task DelayThunk(int time)
{
    TaskAwaiter awaiter = Task.Delay(time).GetAwaiter();
    if (!awaiter.IsCompleted)
        AsyncHelpers.UnsafeAwaitAwaiter(awaiter);
    awaiter.GetResult();
}

however, when this thunk is called we end up capturing the TrackingSynchronizationContext inside the thunk. Eventually that results in the DelayThunk continuation being posted back to that synchronization context, followed by Foo's continuation being moved back to the thread pool by the runtime async infrastructure.

This PR fixes this and makes the thunks transparent in context behavior. At the same time it also optimizes the runtime async -> async1 path to be more efficient in the suspension case: the async1 task now directly invokes the runtime async infrastructure as its continuation, instead of going through multiple layers of indirection.

I also fixed a bug where we could end up invoking the wrong variant of OnCompleted for non-unsafe notifiers if they implemented both INotifyCompletion and ICriticalNotifyCompletion.

I have also renamed ThunkTask -> RuntimeAsyncTask.

This PR is still a WIP as it does not yet handle the ValueTask continuation path.

Fix #119621 (likely...)

Runtime async callable thunks were using `TaskAwaiter` directly, but
that has the normal await semantics which will either post continuations
to the captured synchronization context or to the thread pool. This
introduces an observable behavior change with async1 where sometimes
even configured awaits will end up posting back to a captured
synchronization context.

For example, consider an example like:
```csharp
private static async Task Foo()
{
    SynchronizationContext.SetSynchronizationContext(new TrackingSynchronizationContext());
    await Task.Delay(1000).ConfigureAwait(false);
}
```csharp

Before this change the runtime async call to `Task.Delay` creates a
runtime async callback thunk that roughly looks like
```csharp
Task DelayThunk(int time)
{
    TaskAwaiter awaiter = Task.Delay(time).GetAwaiter();
    if (!await.IsCompleted)
        AsyncHelpers.UnsafeAwaiterAwaiter(awaiter);
    awaiter.GetResult();
}
```
however, when this thunk is called we end up posting back to
`TrackingSynchronizationContext`, before the continuation for `Foo` then
must move its continuation back to the thread pool.

This PR fixes this and makes the thunks transparent in context behavior.
At the same time it also optimizes the runtime async -> async1 path to
be more efficient in the suspension case: the async1 task now directly
invokes the runtime async infrastructure as its continuion, instead of
going through multiple layers of indirection.
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 3, 2025
Comment on lines 704 to 793
//private class RuntimeAsyncContinuation : TaskContinuation, IThreadPoolWorkItem
//{
// private readonly Action _act;
// public RuntimeAsyncContinuation(Action act)
// {
// _act = act;
// }

// public void Execute() => _act();

// internal override Delegate[]? GetDelegateContinuationsForDebugger() => [_act];
// internal override void Run(Task completedTask, bool canInlineContinuationTask)
// {
// if (canInlineContinuationTask)
// {
// ref Task? currentTask = ref Task.t_currentTask;
// Task? prevCurrentTask = currentTask;
// try
// {
// if (prevCurrentTask != null)
// currentTask = null;

// // This should call into RuntimeAsyncTask.MoveNext.
// _act();
// }
// finally
// {
// if (prevCurrentTask != null)
// currentTask = prevCurrentTask;
// }
// }
// else
// {
// // No stack to run RuntimeAsyncTask.MoveNext, which might
// // execute arbitrary continuation code.
// ThreadPool.UnsafeQueueUserWorkItemInternal(this, preferLocal: true);
// }
// }
//}

//internal struct TransparentTaskAwaiter<T> : ICriticalNotifyCompletion
//{
// private readonly Task<T> _task;

// internal TransparentTaskAwaiter(Task<T> task)
// => _task = task;

// public bool IsCompleted => _task.IsCompleted;

// public T GetResult()
// {
// TaskAwaiter.ValidateEnd(_task);
// return _task.ResultOnSuccess;
// }

// public void OnCompleted(Action continuation)
// {
// throw new NotSupportedException();
// }

// public void UnsafeOnCompleted(Action continuation)
// {
// throw new NotSupportedException();
// }
//}

//internal struct TransparentTaskAwaiter : ICriticalNotifyCompletion
//{
// private readonly Task _task;

// internal TransparentTaskAwaiter(Task task)
// => _task = task;

// public bool IsCompleted => _task.IsCompleted;

// public void GetResult()
// {
// TaskAwaiter.ValidateEnd(_task);
// }

// public void OnCompleted(Action continuation)
// {
// throw new NotSupportedException();
// }

// public void UnsafeOnCompleted(Action continuation)
// {
// throw new NotSupportedException();
// }
//}
Copy link
Member Author

Choose a reason for hiding this comment

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

This would be the higher level implementation of this -- basically a new kind of TaskContinuation that never moves the continuation to a different SynchronizationContext. However, it results in more levels of indirection and allocations than is strictly necessary, since we can have Task call into the RuntimeAsyncTask directly as the continuation.

Copy link
Member

Choose a reason for hiding this comment

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

Another possible fix - in e96e206 if our continuation wants to tun on threadpool, we clear the context before calling OnComplete so that OnComplete itself picks a continuation that matches ConfigureAwait(false)

Adding some helpers ito Task infrastructure and calling that could be another option (since we control the Task). I think we may end up doing many changes to Task to make async1-async2 interaction more efficient. It is probably not the time to do a lot of changes like that yet, but if it helps with correctness issues, then why not.

Let me look at the fix.

Comment on lines +143 to 144
public ICriticalNotifyCompletion? CriticalNotifier;
public INotifyCompletion? Notifier;
Copy link
Member Author

Choose a reason for hiding this comment

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

If someone calls AsyncHelpers.AwaitAwaiter, then that should result in call to INotifyCompletion.OnCompleted, but today if the awaiter also implements ICriticalNotifyCompletion it will instead end up calling ICriticalNotifyCompletion.UnsafeOnCompleted. This is a fix for that.

public Continuation? SentinelContinuation;
public ICriticalNotifyCompletion? CriticalNotifier;
public INotifyCompletion? Notifier;
public Task? Task;
Copy link
Member

Choose a reason for hiding this comment

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

And for ValueTask we will store the result of .AsTask ?

@VSadov
Copy link
Member

VSadov commented Oct 6, 2025

I think this approach will work.

I'd like to see ValueTask support. There could be some trickiness there because of IValueTaskSource and stuff related to that.
The docs recommend using .AsTask to do anything complex with ValueTask.

I think we might want to special case "IsCompleted" scenario for valuetask - to not make a Task when none is needed, but the rest can be done via .AsTask

if (valuetask.IsCompleted)
    return valuetask.Result;

Task t = valuetask.AsTask;

< .. same code as for handling Task .. > 

The most common case is ValueTask wraps a value.
Then it may wrap a Task (and AsTask for that is trivial).
Then it may wrap an IValueTaskSource, and that is more complex. Perhaps .AsTask is less likely to run into some quirk or incompatible behavior.

@jakobbotsch
Copy link
Member Author

I think this approach will work.

I'd like to see ValueTask support. There could be some trickiness there because of IValueTaskSource and stuff related to that. The docs recommend using .AsTask to do anything complex with ValueTask.

I think we might want to special case "IsCompleted" scenario for valuetask - to not make a Task when none is needed, but the rest can be done via .AsTask

if (valuetask.IsCompleted)
    return valuetask.Result;

Task t = valuetask.AsTask;

< .. same code as for handling Task .. > 

The most common case is ValueTask wraps a value. Then it may wrap a Task (and AsTask for that is trivial). Then it may wrap an IValueTaskSource, and that is more complex. Perhaps .AsTask is less likely to run into some quirk or incompatible behavior.

We can definitely do it via AsTask, though I do worry that we defeat (some of) the purpose of the ValueTask optimization then. However I think it will be good enough for now -- optimizing that is similar in spirit to #119842, in that the Continuation being created inside the thunk already has all the necessary data. We just need to find a way to generically access it and call OnCompleted. But that would probably need another stub or generic instantiation.

Comment on lines +28 to +32
[Fact]
public static void RuntimeAsyncCallableThunks()
{
RuntimeAsyncCallableThunksAsync().GetAwaiter().GetResult();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Added some tests for runtime async callable thunks for ValueTask methods, since it seemed we had no coverage under the async suite.

@jakobbotsch
Copy link
Member Author

I implemented the ValueTask part now @VSadov. Do you mind testing this on your libraries tests PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners runtime-async
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RuntimeAsync] A few tests fail with runtime async due to unexpected use of sync context
2 participants