-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[RuntimeAsync] Respect IsFlowSuppressed when capturing execution context for suspension. #122125
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
Conversation
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.
Pull request overview
This PR fixes issue #122052 by ensuring that ExecutionContext.IsFlowSuppressed() is properly respected when capturing execution context during async method suspension in the RuntimeAsync feature. The fix aligns the RuntimeAsync suspension code path with the existing behavior in AsyncMethodBuilderCore, ensuring that when execution context flow is suppressed, the captured context is treated as null (default execution context) rather than capturing the current context.
Key changes:
- Modified
CaptureExecutionContext()to checkm_isFlowSuppressedand returnnullwhen flow is suppressed - Changed
ExecutionContext.m_isFlowSuppressedvisibility from private to internal to enable the check - Enabled RuntimeAsync feature by default and removed test build restrictions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs | Added flow suppression check to CaptureExecutionContext() method and clarifying comments to distinguish between contexts captured for synchronous restoration vs. suspension |
| src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs | Changed m_isFlowSuppressed field visibility from private to internal to allow access from AsyncHelpers |
| src/coreclr/inc/clrconfigvalues.h | Enabled RuntimeAsync feature by default (changed from 0 to 1) |
| src/tests/async/Directory.Build.targets | Removed conditional that disabled runtime async testing for non-NativeAOT builds |
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs
Outdated
Show resolved
Hide resolved
jakobbotsch
left a comment
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.
Do we have a targeted test for this?
|
added some test scenarios. @jakobbotsch @davidwrighton - PTAL. Thanks! |
...raries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs
Show resolved
Hide resolved
|
Hold on. There are testcase failures in Libraries now. The result is different if running a callback with an already dirty context. That is why the When context is applied to a thread, |
|
Basically, suppressing context flow will result in callback executing in a nondeterministic context that may depend on implementation details. (i.e. did we switch threads or ran "inline",...) Indeed, the following produces randomly different output: internal class Program
{
static void Main(string[] args)
{
TestNoFlowOuter().GetAwaiter().GetResult();
}
public static AsyncLocal<long?> s_local = new AsyncLocal<long?>();
private static async Task TestNoFlowOuter()
{
s_local.Value = 7;
await TestNoFlowInner();
System.Console.WriteLine(s_local.Value);
}
private static async Task TestNoFlowInner()
{
ExecutionContext.SuppressFlow();
s_local.Value = 42;
// returns synchronously, context stays the same.
await ChangeThenReturn();
System.Console.WriteLine(s_local.Value);
// returns asynchronously, context should not flow.
await ChangeYieldThenReturn();
// The following may print either False or True;
// I see mostly True in Debug, but sometimes False.
// In Release I see False, but would not be surprised if sometimes it prints True
System.Console.WriteLine(s_local.Value == null);
}
[MethodImpl(MethodImplOptions.NoInlining)]
private static async Task ChangeThenReturn()
{
s_local.Value = 123;
}
[MethodImpl(MethodImplOptions.NoInlining)]
private static async Task ChangeYieldThenReturn()
{
s_local.Value = 123;
// restore flow so that state is not cleared by Yield
ExecutionContext.RestoreFlow();
await Task.Yield();
System.Console.WriteLine(s_local.Value);
}
} |
|
Using I'll try using some other sentinel, so that the most common case, when the context is |
|
The failures seem to be: #122228 |
|
failures in Cryptography.Tests.MLKemImplementationTests are #122228 |
This reverts commit 44c1e2b.
...m.Private.CoreLib/src/System/Runtime/CompilerServices/PoolingAsyncValueTaskMethodBuilderT.cs
Show resolved
Hide resolved
|
@stephentoub - Thanks! |
Fixes: #122052
=== async1 counterpart:
Regular/synchronous Capture/Restore of execution context should work unconditionally.
runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilderCore.cs
Lines 30 to 33 in aa500a4
But captures on the suspension code path use
ExecutionContext.Capture, that does not capture if the current context suppresses the flow.runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs
Lines 158 to 167 in aa500a4