Uh oh!
There was an error while loading. Please reload this page.
Fix restore of contexts in async task returning thunks - #119818
Conversation
The previous IL generated approximately implements the following:
```csharp
ExecutionAndSyncBlockStore store = default;
store.Push();
Continuation cont = null;
T result = default;
try
{
try
{
result = Inner(args);
cont = StubHelpers.AsyncCallContinuation();
}
catch (Exception ex)
{
return Task.FromException(ex);
}
}
finally
{
store.Pop();
}
if (cont == null)
return Task.FromResult(result);
return FinalizeTaskReturningThunk(cont);
```
However, since `FinalizeTaskReturningThunk` is where we call
`OnCompleted`, this means that user code gets called after we have
restored contexts.
To fix the issue change the thunks to look approximately like
```csharp
ExecutionAndSyncBlockStore store = default;
store.Push();
try
{
Continuation cont;
try
{
T result = Inner(args);
cont = StubHelpers.AsyncCallContinuation();
if (cont == null)
return Task.FromResult(result);
}
catch (Exception ex)
{
return Task.FromException(ex);
}
return FinalizeTaskReturningThunk(cont);
}
finally
{
store.Pop();
}
```
which ensures that the contexts do not get restored until after
`OnCompleted` has been called.This reverts commit 9c461c5.
jakobbotsch
commented
Sep 17, 2025
Tests passed in the previous run with async tests enabled. |
jakobbotsch
commented
Sep 17, 2025
PTAL @VSadov |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical issue with async task returning thunks where execution contexts were being restored before user code completion callbacks could execute. The fix restructures the IL code generation to ensure OnCompleted callbacks execute before context restoration.
- Moves the finally block containing context restoration to the end of the method
- Reorganizes control flow to ensure completion callbacks execute within the proper context
- Consolidates return logic to use a single return path through a dedicated label
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
VSadov
commented
Sep 18, 2025
The fix makes sense, but when I try running tests locally, I still see I do not see this failure in CI, if I push the same change to the other PR. |
jakobbotsch
commented
Sep 18, 2025
Can you try logging the |
The test actually logs the callstack and fails if the log is not empty. And it dumps the stack into the failure: But we should see this in the lab too, and we do not and that worries me. Otherwise, I do not see effects of this PR. I see the same tests fail before and after. |
VSadov
commented
Sep 18, 2025
It could be a separate issue. Could be something with |
I've run tests on a devbox machine. I do not use it often so it is a relatively clean setup. Maybe it is a separate issue and these tests do not run in the lab for some reason. |
jakobbotsch
commented
Sep 18, 2025
That's odd, at least locally I did see this fixing |
That is another odd thing. I do not see that. (it did not fail before the change) I will try comparing |
VSadov
commented
Sep 18, 2025
Figured this part. |
VSadov
commented
Sep 18, 2025
I think this PR fixes a real issue, which causes But #119621 is caused by something else and we should keep that open. |
Uh oh!
There was an error while loading. Please reload this page.
jakobbotsch
commented
Sep 19, 2025
/backport to release/10.0 |
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17859957716 |
The previous IL generated approximately implements the following:
```csharp
ExecutionAndSyncBlockStore store = default;
store.Push();
Continuation cont = null;
T result = default;
try
{
try
{
result = Inner(args);
cont = StubHelpers.AsyncCallContinuation();
}
catch (Exception ex)
{
return Task.FromException(ex);
}
}
finally
{
store.Pop();
}
if (cont == null)
return Task.FromResult(result);
return FinalizeTaskReturningThunk(cont);
```
However, since `FinalizeTaskReturningThunk` is where we call
`OnCompleted`, this means that user code gets called after we have
restored contexts.
To fix the issue change the thunks to look approximately like
```csharp
ExecutionAndSyncBlockStore store = default;
store.Push();
try
{
Continuation cont;
try
{
T result = Inner(args);
cont = StubHelpers.AsyncCallContinuation();
if (cont == null)
return Task.FromResult(result);
}
catch (Exception ex)
{
return Task.FromException(ex);
}
return FinalizeTaskReturningThunk(cont);
}
finally
{
store.Pop();
}
```
which ensures that the contexts do not get restored until after
`OnCompleted` has been called.
The previous IL generated approximately implements the following:
However, since
FinalizeTaskReturningThunkis where we callOnCompleted, this means that user code gets called after we have restored contexts.To fix the issue change the thunks to look approximately like
which ensures that the contexts do not get restored until after
OnCompletedhas been called.