Start connections on a fresh ExecutionContext

[davidfowl]

• This change removes the ExecutionContext from new connections, this removes the possibility of capturing request scoped async locals and flowing them to the hub. This will ensure that the IHttpContextAccessor and Activity for hub invocations is always null and aren't carried over from the incoming request.
• This will break the ability to pass state to the hub via an async local from middleware but I'm OK with that.

Related to #29846

This breaks the localization middleware for SignalR and for Blazor scenarios

aspnetcore/src/Components/test/testassets/TestServer/InternationalizationStartup.cs

Line 42 in c925f99

app.UseRequestLocalization(options =>

PS: I need to write a test but nothing works in visual studio...

cc @stephentoub you were asking about dotnet/runtime#47998, this is the change.

[stephentoub]

Why do you choose this route rather than:

await AwaitableThreadPool.Yield();

Task t;
using (ExecutionContext.SuppressFlow())
    t = connectionDelegate(this);

await t;

?

[davidfowl]

I generally don't like using suppress flow. I prefer the more explicit call that doesn't capture the ExecutionContext.

[davidfowl]

Also the fact that SuppressFlow throws if the flow is already suppressed makes it annoying. You need to always code around the fact that somebody further up the stack might have already suppressed it.

[stephentoub]

You need to always code around the fact that somebody further up the stack might have already suppressed it.

There's no user code higher up the stack here. You shouldn't need to check or need additional code around it.

I generally don't like using suppress flow

Understood in general. In practice here, though, the code using SuppressFlow would seem to be less convoluted and (I'd need to measure to know for sure) less expensive.

[davidfowl]

There's no user code higher up the stack here. You shouldn't need to check or need additional code around it.

There is. That's where the async locals come from and why this change is happening in the first place.

Understood in general. In practice here, though, the code using SuppressFlow would seem to be less convoluted and (I'd need to measure to know for sure) less expensive.

I think the code does look less convoluted but we use this pattern in other places in ASP.NET Core and it's a stylistic thing. It's the same reason I prefer using *Unsafe APIs instead of suppressing the flow in various cases.

[davidfowl]

Are you saying:

ExecutionContext.SuppressFlow();

await Task.Yield();

ExecutionContext.SuppressFlow();

Works?

[davidfowl]

The other thing I don't like about that is that it's too subtle vs a call that says "This will not capture the execution context" and the code sucks a little bit because. You know what I really want? Task.UnsafeRun. That would work here as well.

[stephentoub]

Are you saying ... works?

Forgetting for the moment the fact that Task.Yield() respects current SynchronizationContext as well (which your AwaitableThreadPool.Yield() does not), yes... suppression itself doesn't flow, and the thread pool ensures a work item can't leak a change to suppression to the next work item.

[halter73]

You know what I really want? Task.UnsafeRun. That would work here as well.

Why do you want to queue to the ThreadPool? I thought this this change was strictly about the ExecutionContext.

[halter73]

I see this change removed the old await AwaitableThreadPool.Yield(); Never mind me.

[pranavkm]

Can you file an issue to track the follow up work on this? I thought one of the tests was failing with this change.

[davidfowl]

I haven't fixed it. I wanted to look at it before merging

[BrennanConroy]

So any logging wont have the connection id associated with it? Do we want to manually add it back?

[davidfowl]

So any logging wont have the connection id associated with it? Do we want to manually add it back?

Which logging exactly? The Kestrel connection id would be lost and the activity would be lost.

[BrennanConroy]

SignalR's connection id we put in the logging scope

aspnetcore/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

Line 319 in 5638880

logScope.ConnectionId = connection?.ConnectionId;

[davidfowl]

SignalR's connection id we put in the logging scope

argh good catch.