Skip to content
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

Retain SynchronizationContext #519

Merged

Conversation

leeoades
Copy link
Contributor

It is recommended to use ConfigureAwait(false) and Task.Run(...) in library code. However, both of these statements lose the SynchronizationContext. There are certain circumstances where it is critical that this is retained, such as within Microsoft Orleans Grains.
I have added a new optional property called RetainSynchronizationContext which defaults to the current behaviour of false. Setting it to true:

  • changes all ConfigureAwait calls to true
  • changes the Task.Run call (which uses the ThreadPool context) to the equivalent Task.Factory.New call, except with the TaskScheduler changed to use the SynchronizationContext.

I wrote unit tests to hit every ConfigureAwait call to prove they were losing the SynchronizationContext before implementing the flag. (I know right, actual red-green development!)

Questions and comments are most welcome. I've tried to make the code and unit tests readable but if there is further clarity that can be achieved, then please suggest away.

@mclift
Copy link
Member

mclift commented Apr 28, 2023

Just an observation—and I haven't got my head around the reasons for this yet—there are two unit tests (ignoring the XUnit ones) that continue to pass when RetainSynchronizationContext is initialised to false. I'd like to understand why this is, and will try to block out some time this weekend to dig into it some more.

@leeoades
Copy link
Contributor Author

there are two unit tests (ignoring the XUnit ones) that continue to pass when RetainSynchronizationContext is initialised to false

Thanks for having a look. It's an area of c# I wasn't completely understanding until recently.

To which unit tests are you referring?

@mclift
Copy link
Member

mclift commented Apr 29, 2023

OnEntry_should_retain_SyncContext and Single_activation_should_retain_SyncContext

@leeoades
Copy link
Contributor Author

Ah I see, yes. So the reason for this is because whilst the sync context has indeed been lost, those particular tests do not go on to make any other calls and as such the effect is not seen.
Single_activation_should_retain_SyncContext was the first test I wrote as part of the exploration and I suppose could be removed to avoid any confusion. As you say, it proves nothing.
Similarly OnEntry_should_retain_SyncContext can be removed in favour of Multiple_OnEntry_should_retain_SyncContext which does surface the issue.
I'll remove both.

@mclift
Copy link
Member

mclift commented Apr 29, 2023

Thanks. I was starting to investigate pre-/post-capture options but like you say it doesn't really prove anything that isn't already covered by the other tests. I've learned a lot about sync context today!

@mclift mclift merged commit 9311ad4 into dotnet-state-machine:dev Apr 29, 2023

private async Task LoseSyncContext()
{
await Task.Delay(TimeSpan.FromMilliseconds(1)).ConfigureAwait(false); // Switch synchronization context and continue

Choose a reason for hiding this comment

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

Note the await Task.Delay(TimeSpan.FromMilliseconds(1)) is not guaranteed to run asynchronously and can, in fact, complete synchronously. In this case, ConfigureAwait(false) won't switch the synchronization context. To guarantee the asynchronous execution, use Task.Yield() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fascinating! Are you able to provide an example of when this would do this - or is it simply system dependant?

Choose a reason for hiding this comment

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

This is just an implementation detail of async/await. Any awaited code can finish synchronously (this is documented somewhere, I'll try to find the link).

Choose a reason for hiding this comment

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

Sorry, I can't find the documentation. But I will try to prove it with the source code. Look into the Task.Delay implementation. It returns DelayPromise instance. Its constructor has the following code with a comment

if (IsCompleted)
{
    // Handle rare race condition where the timer fires prior to our having stored it into the field, in which case
    // the timer won't have been cleaned up appropriately.  This call to close might race with the Cleanup call to Close,
    // but Close is thread-safe and will be a nop if it's already been closed.
    _timer.Dispose();
}

It means the task can complete before it even was constructed. It means you are going to await the completed task. await'ing task is essentially calling GetAwaiter().GetResult(). Check the TaskAwaiter.GetResult() implementation

public void GetResult()
{
    ValidateEnd(m_task);
}

internal static void ValidateEnd(Task task)
{
    // Fast checks that can be inlined.
    if (task.IsWaitNotificationEnabledOrNotRanToCompletion)
    {
        // If either the end await bit is set or we're not completed successfully,
        // fall back to the slower path.
        HandleNonSuccessAndDebuggerNotification(task);
    }
}

And IsWaitNotificationEnabledOrNotRanToCompletion implementation

internal bool IsWaitNotificationEnabledOrNotRanToCompletion
{
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    get
    {
        return (m_stateFlags & (Task.TASK_STATE_WAIT_COMPLETION_NOTIFICATION | Task.TASK_STATE_RAN_TO_COMPLETION))
                != Task.TASK_STATE_RAN_TO_COMPLETION;
    }
}

Since the task state is TASK_STATE_RAN_TO_COMPLETION, the IsWaitNotificationEnabledOrNotRanToCompletion method will return false. ValidateEnd and GetResult will return immediately on the same thread, and the context won't switch.

The Task.Delay is just an example. Any fast enough async method can complete before you await it, and GetResut() will return synchronously.

Choose a reason for hiding this comment

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

await'ing task is essentially calling GetAwaiter().GetResult()

It's a rude simplification. In fact, there is an async state machine

private void MoveNext()
{
    int num = <>1__state;
    try
    {
        TaskAwaiter awaiter;
        if (num != 0) // num = -1 on the first execution
        {
            awaiter = Task.Delay(TimeSpan.FromMilliseconds(1)).GetAwaiter();
            if (!awaiter.IsCompleted)
            {
                ...
                return;
            }
        }
        else
        {
            ...
        }
        awaiter.GetResult();
    }
    catch (Exception exception)
    {
        ...
    }
}

Since the awaiter.IsCompleted is true we are jumping straight to the awaiter.GetResult().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for taking the time to explain it! Well, I've been learning so much recently! I completely agree- I'll stick a Task.Yield in there instead.

@leeoades
Copy link
Contributor Author

@gao-artur - I tried to utilise Task.Yield - but it seems that its function is to return the execution path to the current context, which is the opposite of what I need it to do - I'm trying to guarantee that I'm losing the context. Any ideas?

@leeoades
Copy link
Contributor Author

How about

await Task.Run(() => { }).ConfigureAwait(false);

The entire function of Task.Run is to schedule the action on the ThreadPool so we're guaranteed to be in a different sync context?

@leeoades
Copy link
Contributor Author

leeoades commented Apr 30, 2023

@gao-artur - I've implemented this change. Thanks for pointing it out!
Please see: #528

@gao-artur
Copy link

@gao-artur - I tried to utilise Task.Yield - but it seems that its function is to return the execution path to the current context, which is the opposite of what I need it to do - I'm trying to guarantee that I'm losing the context. Any ideas?

It seems I have to learn more, too 🙂. You are right, the Task.Yield() returns to the original context, and the documentation says it explicitly.

How about

await Task.Run(() => { }).ConfigureAwait(false);

The entire function of Task.Run is to schedule the action on the ThreadPool so we're guaranteed to be in a different sync context?

Indeed, Task.Run should do the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants