Make HttpContextAccessor static AsyncLocal

[Tratcher]

#723 The .NET 4.5.1 version is static but the .NETStandard1.3 version isn't, which can cause problems if you end up with different instances of HttpContextAccessor.

[Tratcher]

@dotnetjunkie

[davidfowl]

Part of me doesn't like this change. The http context accessor should be a singleton, newing it up now shares the async local state with all other instances.

@dotnetjunkie Remind me how you ran into this.

[dotnetjunkie]

@davidfowl, try not to act purely on feelings. That typically not works out well :)

Here's a simple way to reproduce the issue:

services.AddTransient<IHttpContextAccessor, HttpContextAccessor>();

[davidfowl]

The inconsistency is an issue but I'd rather make .NET Framework act like .NET Core than vice versa. Adding a transient IHttpContextAccessor is just broken.

[dotnetjunkie]

Yes, making that component transient is clearly broken, but by no way does the framework guide developers into the right direction.

It is really easy for a user to do the wrong thing, while it is really hard for the user to track down the bug that is the result of this. I already shown two completely different scenarios (both in the previous comment and in #723) that can cause bugs.

[davidfowl]

Agreed but that can be solved in other ways. I'm just of the opinion this is the wrong fix for this particular problem. There are been some huge improvements made to the allocation profile to async locals in .NET Core vnext so maybe it's time to turn this back on by default again and measure. Also dropping .NET 4.5.1 (and rebasing to .NET Framework 4.6) support would let us use async local on both .NET Core and .NET Framework.

That solves both the lifetime issue and saves the implementation from hard coding statics.

[dotnetjunkie]

The only actual fix here is to make the AsyncLocal field static, because it's very easy for a user to accidentally create a second instance of the HttpContextAccessor as explained in #723.

[davidfowl]

The only actual fix here is to make the AsyncLocal field static, because it's very easy for a user to accidentally create a second instance of the HttpContextAccessor as explained in #723.

I'm not convinced that would be common if we were able to add this service back to hosting by default.

[davidfowl]

I mentioned it here #753 (comment)

[Tratcher]

As a data point, we have many uses of AsyncLocal and most of them are static.
https://github.com/search?q=org%3Aaspnet+AsyncLocal&type=Code

@davidfowl how much of your concern applies to these other cases?