Skip to content
This repository has been archived by the owner. It is now read-only.

Make sure IHttpContextAccessor is always registered #728

Closed
dotnetjunkie opened this issue Oct 16, 2016 · 27 comments

Comments

@dotnetjunkie
Copy link

commented Oct 16, 2016

The IHttpContextAccessor allows access to the current executing HttpContext and seems to be a core component of ASP.NET. Still however, the IHttpContextAccessor is not registered by default by ASP.NET while it IMO definitely should.

The whole idea behind the new configuration model in ASP.NET is that everything is registered for you magically behind the covers, but even if you call AddMvc or even AddMvcCore the IHttpContextAccessor isn't registered, while there are many integration scenarios that require its availability. This means that 3rd parties (or possibly even future framework components) always need to check its availability and throw an exception when it's not registered and application developers always need to register it manually.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

The whole idea behind the new configuration model in ASP.NET is that everything is registered for you magically behind the covers, but even if you call AddMvc or even AddMvcCore the IHttpContextAccessor isn't registered, while there are many integration scenarios that require its availability.

Not everything, everything that is required. We did extra work to make sure that our own types don't rely on it (because ambient state is bad and hard to reason about).

One of the reasons we took it out for performance. We don't want every ASP.NET application to enable execution context copies (what happens as soon as you use async local). Also, we don't really want users to use this type unless they need it.

This means that 3rd parties (or possibly even future framework components) always need to check its availability and throw an exception when it's not registered and application developers always need to register it manually.

Add it if you need it in your services. That's the pattern we use. https://github.com/aspnet/Identity/blob/ee83dfe9be3dc17fa8367a0031ff355392db8a76/src/Microsoft.AspNetCore.Identity/IdentityServiceCollectionExtensions.cs#L54

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

Though to be honest, if nobody resolves it, there should be no cost. We did all of the hard work to decouple ourselves from it so maybe it's ok to add it to hosting.

/cc @rynowak @Tratcher

@benaadams

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2016

If you register it then HttpContextFactory will resolve it and set the AsyncLocal to the HttpContext; which means every await and dispatch to threadpool is no longer on the default execution context fast path.

@dotnetjunkie

This comment has been minimized.

Copy link
Author

commented Oct 16, 2016

One of the reasons we took it out for performance.

I find it hard to believe that the use of such ambient state will actually cause any detectable performance hit. Especially with all the I/O going on. Have you actually measured this and publically described this somewhere what the performance impact is?

we don't really want users to use this type unless they need it.

I don't believe that adding this by default will cause users to automatically use it. I just think that not adding it by default will simply cause users to add it by hand.

because ambient state is bad

Apparently not that bad, because ASP.NET uses it for logging scopes.

every await and dispatch to threadpool is no longer on the default execution context fast path.

Since everybody uses logging in their application, doesn't this mean that we never run on this context fast path?

@benaadams

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2016

because ambient state is bad

Apparently not that bad, because ASP.NET uses it for logging scopes.

Only ConsoleLogScope alters the execution context in this way; and if want high throughput you shouldn't be really using that in production as then your entire app is gated on one global lock in Console if you log anything.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

Ah yes that was the problem with turning it on by default. We need to resolve it ourselves to set it. It needs to be added by the components that need it.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

I find it hard to believe that the use of such Ambient Context will actually cause any detectable performance hit. Especially with all the I/O going on. Have you actually measured this and publically described this somewhere what the performance impact is?

Yes, @benaadams measured it as part of our benchmarks runs and it was significant. Scoped logging does tend to use async local as well so the cost might be incurred there depending on the logging system. These things are fine, they just can't be all on by default.

@dotnetjunkie

This comment has been minimized.

Copy link
Author

commented Oct 16, 2016

Yes, @benaadams measured it as part of our benchmarks runs and it was significant.

Can you link to his benchmark results?

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

This was the change that disabled it.

Can't find any performance numbers on the repo but you might be able to the differences in the excel spreadsheet in the Updated Results commit after the change:

https://github.com/aspnet/benchmarks/commits/dev?after=G6n%2FgPpHkWvHSt0NmAuyU1ZqXqIrMTM5

@benaadams

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2016

Have you actually measured this and publically described this somewhere what the performance impact is?

aspnet/Hosting#330
aspnet/Hosting#356

On 4.6+ it allocates 680k per 2000 requests and creates an additional 2 dictionaries per request.

Every await that is async allocates an additional 8 bytes dotnet/coreclr#3157

When the HttpContext is set ExecutionContext.SetLocalValue is additionally run

@benaadams

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2016

You don't normally need to use IHttpContextAccessor as DI will inject the HttpContext for you if you ask for it; and then is scope is more controlled (as you shouldn't be using it outside of a request)

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

You don't normally need ambient state. It shouldn't be the default way of doing things. This is an optional convenience for people. Unfortunately it's required to get the scoped service provider for scenarios like this aspnet/Mvc#5403

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

Moar perf numbers - aspnet/Mvc#3523

@dotnetjunkie

This comment has been minimized.

Copy link
Author

commented Oct 16, 2016

What I see is that the there is especially a perf hit under .NET 4.5.1 where there is no optimized AsyncLocal. Besides that, we will see the improvement be deminished when any I/O is done. But since this optimization is a very conscious action, I understand that you won't go to the 'default on' scenario. It's unfortunate though that this makes less intuitive for non-conformers.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 16, 2016

@dotnetjunkie It shouldn't be exposed if you add the right services. services.AddSimpleInjectorServices()

@dotnetjunkie

This comment has been minimized.

Copy link
Author

commented Oct 16, 2016

It shouldn't be exposed if you add the right services. services.AddSimpleInjectorServices()

@davidfowl that's a discussion for a different thread.

And for everybody who is lead to believe that ambient state is bad, please read this discussion.

@Metamorphus

This comment has been minimized.

Copy link

commented May 12, 2017

OK, but what if I need access to HttpContext inside my class library. I'll have to oblige all the developers using it to register HttpContextAccessor? My library is for web applications, so it should be able to get the request domain.

P.S. Passing HttpContext from client code's contorollers to my class isn't an option either: clients can create web applications without MVC.

@dotnetjunkie

This comment has been minimized.

Copy link
Author

commented May 12, 2017

You don't normally need to use IHttpContextAccessor as DI will inject the HttpContext for you if you ask for it; and then is scope is more controlled (as you shouldn't be using it outside of a request)

@benaadams, injecting HttpContext directly into a component's constructor is a bad idea, because HttpContext is runtime data and runtime data should not be injected into application components during construction.

It's for this very reason that Identity Framework fixed a bug prevented the SignInManager from taking HttpContext as constructor argument.

@Tratcher

This comment has been minimized.

Copy link
Member

commented May 12, 2017

P.S. Passing HttpContext from client code's contorollers to my class isn't an option either: clients can create web applications without MVC.

How do client's call your library? HttpContext is not part of MVC, it's part of the underlying ASP.NET Core framework.

@Metamorphus

This comment has been minimized.

Copy link

commented May 12, 2017

Sorry, my library needs request domain just to check its license (license keys are bound to domains). Its functionality doesn't depend on ASP.NET.

@benaadams

This comment has been minimized.

Copy link
Contributor

commented May 12, 2017

@Metamorphus add an extension method for your product that people call in ConfigureServices:

public static class MyProductServiceCollectionExtensions
{
    public static void AddMyProduct(this IServiceCollection services)
    {
        services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>();
    }
}
@dotnetjunkie

This comment has been minimized.

Copy link
Author

commented Oct 24, 2017

It's great to see that the ASP.NET team changed their mind with ASP.NET Core 2.0 and IHttpContextAccessor is now registered by default.

Big thumbs up to the team!

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 25, 2017

@dotnetjunkie it's not really registered by default. It depends on what template you use... 😄

@dotnetjunkie

This comment has been minimized.

Copy link
Author

commented Oct 25, 2017

@davidfowl Can you give more information about this? What do you mean by template? Are you referring to Visual Studio templates? As far as I can tell, the IHttpContextAccessor is added to the IServiceCollection even before ConfigureServices is called. So what influences the addition of this service?

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 25, 2017

Its likely application insights adding it for their own usage. There's a new feature in the WebHost that lets various hosting environments inject application logic. Visual Studio injects the application insights logic in debug only and that's likely what you're seeing.

@dotnetjunkie

This comment has been minimized.

Copy link
Author

commented Oct 25, 2017

I can confirm that when ran outside of Visual Studio, the IHttpContextAccessor registration is missing, while when running from Visual Studio, it actually is registered.

Could you be more specific and describe what exactly is adding the IHttpContextAccessor registration?

I do think that it is a pretty bad idea to have such central abstraction registered conditionally. This can easily lead developers dazzled, since the application runs on their machine, but breaks once deployed.

@khellang

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.