Skip to content

Potential promise context lost fix #4583 #4584

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.

Sign up for GitHub

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

Open
Leinnan wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Potential promise context lost fix #4583 #4584

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70d397d
Potential promise context lost fix
Leinnan Apr 24, 2026
eb680ca
Merge branch 'main' into fix/promiseFix
Leinnan Aug 26, 2026
e5fe74e
Apply feedback
Leinnan Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 60 additions & 11 deletions cli/beamable.common/Runtime/Promise.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,19 @@ void ICriticalNotifyCompletion.UnsafeOnCompleted(Action continuation)

void INotifyCompletion.OnCompleted(Action continuation)
{
((ICriticalNotifyCompletion)this).UnsafeOnCompleted(continuation);
// Mirror TaskAwaiter.OnCompleted: flow ExecutionContext so AsyncLocal
// values set by the caller survive the await and are restored on the
// thread that resolves the promise.
var capturedContext = System.Threading.ExecutionContext.Capture();
if (capturedContext == null)
{
((ICriticalNotifyCompletion)this).UnsafeOnCompleted(continuation);
return;
}
((ICriticalNotifyCompletion)this).UnsafeOnCompleted(() =>
{
System.Threading.ExecutionContext.Run(capturedContext, s => ((Action)s)(), continuation);
});
Comment thread
Leinnan marked this conversation as resolved.
}

/// <summary>
Expand Down Expand Up @@ -1253,6 +1265,18 @@ public void SetStateMachine(IAsyncStateMachine machine)
_stateMachine = machine;
}

private void MoveNextWithCapturedContext(System.Threading.ExecutionContext capturedContext)
{
if (capturedContext == null)
{
_stateMachine.MoveNext();
}
else
{
System.Threading.ExecutionContext.Run(capturedContext, s => ((IAsyncStateMachine)s).MoveNext(), _stateMachine);
}

Copilot AI Apr 28, 2026

Copy link

Choose a reason for hiding this comment

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

This helper uses System.Threading.ExecutionContext, which conflicts with the existing DISABLE_THREADING compilation mode (defined for UNITY_WEBGL at the top of this file) that is meant to avoid System.Threading APIs. Please gate ExecutionContext usage behind #if !DISABLE_THREADING (or equivalent) and keep a non-threading fallback that just calls _stateMachine.MoveNext() so builds with DISABLE_THREADING still compile.

Copilot uses AI. Check for mistakes.
}

public void AwaitOnCompleted<TAwaiter, TStateMachine>(
ref TAwaiter awaiter, ref TStateMachine stateMachine)
where TAwaiter : INotifyCompletion
Expand All @@ -1264,18 +1288,26 @@ public void AwaitOnCompleted<TAwaiter, TStateMachine>(
_stateMachine.SetStateMachine(stateMachine);
}

awaiter.OnCompleted(() =>
{
_stateMachine.MoveNext();
});
// Mirror AsyncTaskMethodBuilder: capture ExecutionContext at the await
// point and restore it before resuming the state machine so AsyncLocal
// values flow across the await.
var capturedContext = System.Threading.ExecutionContext.Capture();
awaiter.OnCompleted(() => MoveNextWithCapturedContext(capturedContext));

Copilot AI Apr 28, 2026

Copy link

Choose a reason for hiding this comment

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

This change alters core await/continuation behavior (ExecutionContext/AsyncLocal flow) but there doesn’t appear to be a unit test that asserts AsyncLocal (or logging scope) values survive await Promise boundaries. Given there are existing PromiseTests, please add a focused test that sets an AsyncLocal value, awaits a Promise that completes on a different callback, and verifies the value is present after resumption (and ideally that suppression/clearing behaves as expected).

Copilot uses AI. Check for mistakes.
}

public void AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(
ref TAwaiter awaiter, ref TStateMachine stateMachine)
where TAwaiter : ICriticalNotifyCompletion
where TStateMachine : IAsyncStateMachine
{
AwaitOnCompleted(ref awaiter, ref stateMachine);
if (_stateMachine == null)
{
_stateMachine = stateMachine;
_stateMachine.SetStateMachine(stateMachine);
}

var capturedContext = System.Threading.ExecutionContext.Capture();
awaiter.UnsafeOnCompleted(() => MoveNextWithCapturedContext(capturedContext));

Copilot AI Apr 28, 2026

Copy link

Choose a reason for hiding this comment

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

System.Threading.ExecutionContext is captured/used in the async method builder here, but this file supports a DISABLE_THREADING build mode (notably for WebGL) where System.Threading APIs are avoided. Please add conditional compilation so the builder still compiles when DISABLE_THREADING is defined (eg, skip capture and resume the state machine directly).

Copilot uses AI. Check for mistakes.
}

public void Start<TStateMachine>(ref TStateMachine stateMachine)
Expand Down Expand Up @@ -1313,6 +1345,18 @@ public void SetStateMachine(IAsyncStateMachine machine)
_stateMachine = machine;
}

private void MoveNextWithCapturedContext(System.Threading.ExecutionContext capturedContext)
{
if (capturedContext == null)
{
_stateMachine.MoveNext();
}
else
{
System.Threading.ExecutionContext.Run(capturedContext, s => ((IAsyncStateMachine)s).MoveNext(), _stateMachine);
}
}

public void AwaitOnCompleted<TAwaiter, TStateMachine>(
ref TAwaiter awaiter, ref TStateMachine stateMachine)
where TAwaiter : INotifyCompletion
Expand All @@ -1324,18 +1368,23 @@ public void AwaitOnCompleted<TAwaiter, TStateMachine>(
_stateMachine.SetStateMachine(stateMachine);
}

awaiter.OnCompleted(() =>
{
_stateMachine.MoveNext();
});
var capturedContext = System.Threading.ExecutionContext.Capture();
awaiter.OnCompleted(() => MoveNextWithCapturedContext(capturedContext));
}

public void AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(
ref TAwaiter awaiter, ref TStateMachine stateMachine)
where TAwaiter : ICriticalNotifyCompletion
where TStateMachine : IAsyncStateMachine
{
AwaitOnCompleted(ref awaiter, ref stateMachine);
if (_stateMachine == null)
{
_stateMachine = stateMachine;
_stateMachine.SetStateMachine(stateMachine);
}

var capturedContext = System.Threading.ExecutionContext.Capture();
awaiter.UnsafeOnCompleted(() => MoveNextWithCapturedContext(capturedContext));

Copilot AI Apr 28, 2026

Copy link

Choose a reason for hiding this comment

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

Same ExecutionContext dependency exists in the non-generic async method builder; without conditional compilation this can break DISABLE_THREADING targets (eg UNITY_WEBGL). Please apply the same #if !DISABLE_THREADING guard/fallback here as well so this code remains portable across supported Unity platforms.

Copilot uses AI. Check for mistakes.
}

public void Start<TStateMachine>(ref TStateMachine stateMachine)
Expand Down
Loading