Use ExecutionContext.Restore rather than EC.Run callback

[benaadams]

Only running inline needs to do extra work and error handling for post run restore

@stephentoub this would be the change for ManualResetValueTaskSourceCore to use ExecutionContext.Restore rather than a generic ref passing callback based ExecutionContext.Run

[stephentoub]

Thanks, @benaadams. Any perf impact?

[benaadams]

Moved to using same api as would use externally.

Only issue/question is undoing SynchronizationContext changes for running inline #37942 (comment)

Resolved

[benaadams]

Any perf impact?

For the common path (with no EC) can only see how it would get a little faster since the calls are inlined for that path.

If InvokeContinuationInline was inline (is an uncommon EC path) rather than a separate method it might cause an issue due to bringing the try/catch block into the method.

[benaadams]

Rebased

[stephentoub]

LGTM other than my one comment. @kouvel, any concerns?

[kouvel]

Would it make sense to call this method Apply or something similar instead of Restore given how it's used?

[benaadams]

Depends on results of api review #38011

[kouvel]

Why would the ASP.NET use case of the public API not use EC.Run() instead? In any case even for a public API (which would be a somewhat advanced API that may be misused), Apply seems more appropriate to me, maybe something to consider in the API review

[stephentoub]

Why would the ASP.NET use case of the public API not use EC.Run() instead?

Because EC.Run forces you into a callback model, which is problematic if the code you're wrapping needs to be async.

[kouvel]

Hmm ok. Would a RunAsync instead be workable?

[stephentoub]

It's not just the shape of the method: it's the overhead. A goal here is to avoid the extra machinery/allocation associated with another level of async method / state machine.

[kouvel]

Makes sense, but if that's why we're exposing the API (for perf), then would it make sense to prefix the API name with Unsafe or something else similarly since it feels like it could be easily misused?

[benaadams]

Hmm ok. Would a RunAsync instead be workable?

Well just putting it behind a regular async method gives that effect.

The ASP.NET case is inlining all the awaits into a top level request loop (as its allocated once for all requests on the connection); however the loop calls user code, if any of the methods are not async (Task.CompletedTask returning), but also change AsyncLocal<T>s then they are not undone and those changes leak into the next request (dotnet/aspnetcore#15384, dotnet/aspnetcore#13991)

The .Restore method allows the EC to be captured as a hoisted var outside the loop; and then reset back at the end of each loop iteration so each new request starts on a fresh ExecutionContext throwing away any changes from the last iteration dotnet/aspnetcore#23175 (kinda like the ThreadPool does)

[benaadams]

e.g.

var ec = ExecutionContext.Capture();
while (!cancelled)
{
    await NextRequest();

    await OnStartAsync(); // User code

    await ProcessRequestAsync(); // User code

    await OnCompleteAsync(); // User code

    ExecutionContext.Restore(ec);
}

[benaadams]

Rebased to retrigger tests

[benaadams]

Good to merge?

[kouvel]

Thanks!