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

[Announcement] The IHttpContextAccessor service is not registered by default #793

Closed
Tratcher opened this issue Jun 8, 2016 · 38 comments
Closed
Labels
Milestone

Comments

@Tratcher
Copy link
Member

@Tratcher Tratcher commented Jun 8, 2016

IHttpContextAccessor can be used to access the HttpContext for the current thread. However, maintaining this state has non-trivial performance costs so it has been removed from the default set of services. Developers that depend on it can add it back as needed:
services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();

RE: aspnet/HttpAbstractions#473

@aminebizid
Copy link

@aminebizid aminebizid commented Jun 9, 2016

So of we don't inject httpcontext we won't have Access to User in our controllers ?

@Tratcher
Copy link
Member Author

@Tratcher Tratcher commented Jun 9, 2016

@zigzag95 IHttpContextAccessor is only intended for accessing the HttpContext in locations where it's not directly available. HttpContext and User are directly available in MVC Controllers so you do not need IHttpContextAccessor.

@tugberkugurlu
Copy link

@tugberkugurlu tugberkugurlu commented Jun 9, 2016

In that sample, you are registering it as a singleton. Shouldn't it be a scoped instance?

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Jun 9, 2016

services.AddIdentity registers this anyway.

https://github.com/aspnet/Identity/blob/79dbed5a924e96a22b23ae6c84731e0ac806c2b5/src/Microsoft.AspNetCore.Identity/IdentityServiceCollectionExtensions.cs#L54

A lot of applications will need identity so this change is confusing as if you use identity you get IHttpContextAccessor but if you don't it doesn't.
So if using identity you get IHttpContextAccessor 'magically'.
Even though HttpConext is available in controller I still want to avoid new on my objects and prefer DI.

Now it is not clear when you need to register IHttpContextAccessor.
What would be the result of registering it twice.

ie

services.AddIdentity<ApplicationUser, IdentityRole>()
                .AddEntityFrameworkStores<ApplicationDbContext>()
                .AddDefaultTokenProviders();
            services.Configure<IdentityOptions>(options =>
            {
                options.Cookies.ApplicationCookie.LoginPath = new PathString("/default/Account/Login/");
            });

            services.AddMvc();
            services.AddAuthorization();
            services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();

The above code runs fine but is there a performance hit to AddIdentity reqistering IHttpContextAccessor and registering it myself.
I would prefer to register it explicitly as it is clearer to my code.

@davidfowl
Copy link
Member

@davidfowl davidfowl commented Jun 9, 2016

The above code runs fine but is there a performance hit to AddIdentity reqistering IHttpContextAccessor and registering it myself.

Performance hit? Is that speculation or did you measure?

In that sample, you are registering it as a singleton. Shouldn't it be a scoped instance?

It's fine being a singleton because the backing store is async local.

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Jun 9, 2016

@davidfowl

Performance hit? Is that speculation or did you measure?

Speculation/Question

I am just not sure of the mechanics of registering it again.

AddIdentity is already providing an instance of HttpContextAccessor but I prefer to explicitly register it.
I don't perceive any performance hit (I didn't notice when IHttpContextAccessor instatiation was removed from Hosting Either :-).

I just wanted to make sure there wasn't a problem registering it again.

@davidfowl
Copy link
Member

@davidfowl davidfowl commented Jun 9, 2016

There is no problem registering it again and certainly no measurable performance hit.

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Jun 9, 2016

@Tratcher [edit] sorry Elion wrong person re the advice to use

services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();

Would it not be better to advise using the TryAdd version

services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>();

since it may not be clear that IHttpContextAccessor may already be registered by another service and we don't need 2 instances.

That way if IHttpContextAccessor is already registered by another service eg AddIdentity a duplicate is not created. Other aspnet services use the TryAdd version for this reason I believe.
See my discussion above with @davidfowl

@brockallen
Copy link

@brockallen brockallen commented Jun 9, 2016

In that sample, you are registering it as a singleton. Shouldn't it be a scoped instance?

It's fine being a singleton because the backing store is async local.

Actually if you register it as a Transient on .NET Core then it doesn't work properly since the implementation for .NET Core is using a AsyncLocal which relies upon the instance variable to track the TLS storage slot. So it has to be registered as a singleton on .NET Core.

@Tratcher
Copy link
Member Author

@Tratcher Tratcher commented Jun 9, 2016

@mikes-gh Sure, TryAdd is slightly better, but shouldn't affect functionality or perf.

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Jun 9, 2016

@Tratcher I may not be understanding the mechanics but if there is no performance impact to registering a second instance why was the default instance removed?

@Tratcher
Copy link
Member Author

@Tratcher Tratcher commented Jun 9, 2016

Only one registered instance gets used so there is a perf difference between 0 and 1, but not between 1 and 2.

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Jun 9, 2016

Ok got it thanks . I still think the announcement should sugest TryAdd though.

@brockallen
Copy link

@brockallen brockallen commented Jun 9, 2016

I still think the announcement should sugest TryAdd though.

It doesn't matter -- it must be a singleton, so doing additional DI config with different lifetimes won't work anyway.

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Jun 10, 2016

@brockallen
TryAddSingleton is the same lifetime as AddSingleton. The difference is TryAdd version checks for the existence of the service desciptor in the service collection and any only adds it if it's not already there. See Dependency Injection repo for the code.

https://github.com/aspnet/DependencyInjection/blob/60ca512beec3fefb4cd377ede009ff96b4848456/src/Microsoft.Extensions.DependencyInjection.Abstractions/Extensions/ServiceCollectionDescriptorExtensions.cs#L85

@brockallen
Copy link

@brockallen brockallen commented Jun 11, 2016

@brockallen TryAddSingleton is the same lifetime as AddSingleton.

This distinction wasn't my point. My point was that the class (either by design or by accident) will only work as a singleton.

dazinator added a commit to dazinator/jsnlog that referenced this issue Jun 17, 2016
@ericwj
Copy link
Contributor

@ericwj ericwj commented Jun 21, 2016

Conceptually the DI provider is a typed dictionary with the service type as the key. So any service can be registered in a for loop a thousand times, it won't have much effect apart from startup performance - which is negligible unless you actually do use a for loop.

I believe the scoping of any service is fixed once it is registered for the first time, I believe that's true for the implementation type as well - correct me if I'm wrong. I recall @DamianEdwards fixed a bug with caching on stage at a conference (//Build?) by simply registering a singleton before registering EF - which would register the same service type as transient. I'm just too lazy to test it right now, it'll probably take about two minutes.

But if the implementation type nor the scoping can't be changed for a service type after registering it for the first time, one could ask why there exists TryAdd overloads at all.

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Jun 21, 2016

Or maybe all service registering should be using TryAdd logic.
Drop TryAdd and move the uniqueness check to Add method.
That would make sure the service collection correctly matches what will be instantiated. That way the useless entries never make it into the collection..

@sandorfr
Copy link

@sandorfr sandorfr commented Jun 21, 2016

This announcement creates a very awkward feeling. The way it's stated gives the impression that we should not use it. But, identity is using it, so a vast majority of apps developed with aspnetcore will depend on it, so I guess (hope) that the cost of this is not that bad or there may be something extremely wrong with identity implementation.

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Jun 22, 2016

non-trivial performance costs

@Tratcher The above is the justification for removal of IHttpContextAccessor by default.
Yet as @sandorfr points out it will be in a LOT of web apps due to identity requiring it.

Should we be concerned about performance in all apps that use identity?
This post does sound like it is discouraging use of IHttpContextAccessor but I'm not sure what to use as an alternative.

@Tratcher
Copy link
Member Author

@Tratcher Tratcher commented Jun 22, 2016

@mikes-gh one of the major tenants of Asp.Net Core is pay-for-play, e.g. you only pay the costs of the components you use. Yes, many apps will use Identity and those apps will have higher costs than apps that don't (and not only because of IHttpContextAccessor). The added cost is not prohibitive, just it's unnecessary for other kinds of applications.

@sandorfr
Copy link

@sandorfr sandorfr commented Jun 22, 2016

The added cost is not prohibitive

Thanks for clarifying that :).

@davidfowl davidfowl changed the title [Anouncment] The IHttpContextAccessor service is not registered by default [Announcment] The IHttpContextAccessor service is not registered by default Jun 24, 2016
@muratg
Copy link

@muratg muratg commented May 12, 2017

We are closing this issue because no further action is planned for this issue. If you still have any issues or questions, please log a new issue with any additional details that you have.

@AndreiRinea
Copy link

@AndreiRinea AndreiRinea commented Nov 9, 2017

The HttpContextAccessor seems to be registered by default in .NET Core 2.0.2. This is partially shown in my issue on StackOverflow.

@Tratcher
Copy link
Member Author

@Tratcher Tratcher commented Nov 9, 2017

No, the stack does not register it by default, but one of your dependencies may. For example ApplicationInsights registers it.

@ahmetsevgili
Copy link

@ahmetsevgili ahmetsevgili commented Dec 5, 2017

It is registered in debug mode but in publish mode it is not registered. What is the differences between them?

@promanchenko-softheme
Copy link

@promanchenko-softheme promanchenko-softheme commented Dec 6, 2017

@ahmetsevgili I have the same problem. Have you already solved it?

Help me, please

@Tratcher
Copy link
Member Author

@Tratcher Tratcher commented Dec 6, 2017

That may be ApplicationInsights only registering it when enabled.

@DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Dec 6, 2017

Application Insights is automatically registered in your application when you launch it under the debugger in Visual Studio, such that you can see & search events coming from your application while debugging in the VS Application Insights search experience. This is the most likely cause of the IHttpContextAccessor being available in debug but not after the application is published. If you need it, please register is explicitly yourself in ConfigureServices.

@ahmetsevgili
Copy link

@ahmetsevgili ahmetsevgili commented Dec 7, 2017

@promanchenko-softheme, use like @Tratcher said that above. Both debug and publish mode work fine after adding
services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();
line

my ConfigureServices snippet like below

public void ConfigureServices(IServiceCollection services)
        {
            services.AddMvc();
            services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();
@muratg muratg changed the title [Announcment] The IHttpContextAccessor service is not registered by default [Announcement] The IHttpContextAccessor service is not registered by default Dec 7, 2017
@migajek
Copy link

@migajek migajek commented Dec 22, 2017

Visual Studio debugger registering IHttpContextAccessor is extremely misleading. It took me a while to find out what the real cause of my "bug" was

@chrispickford
Copy link

@chrispickford chrispickford commented Apr 12, 2018

Experienced this issue in a slightly different context to what's already been posted. When publishing an ASP.NET Core 2 API project via our CI process to an IIS server the following exception is raised:

InvalidOperationException: Unable to resolve service for type 'Microsoft.AspNetCore.Http.IHttpContextAccessor'...

Locally, we're debugging using Kestral and the exception doesn't raise.

Adding services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>(); to the RegisterServices(IServiceCollection) method has resolved the issue in IIS, but I'm curious why this isn't an issue when self-hosting?

@davidfowl
Copy link
Member

@davidfowl davidfowl commented Apr 12, 2018

That's going to be fixed in 2.1, it has to do with the fact that application insights was being injected in visual studio by default and adding those services but only when debugging. We're making some adjustments so we don't have a different debug/non-debug experience by default.

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Apr 12, 2018

So does that mean adding back HttpContextAccessor service by default?

@davidfowl
Copy link
Member

@davidfowl davidfowl commented Apr 12, 2018

So does that mean adding back HttpContextAccessor service by default?

No

@mikes-gh
Copy link

@mikes-gh mikes-gh commented Apr 12, 2018

@davidfowl

No

How is it solved then? Code analyzer to catch the dependency in debug build? or.....

@springy76
Copy link

@springy76 springy76 commented Apr 13, 2018

As HttpContextAccessor is just a wrapper around a single private static field I see no reason why it should make any difference anywhere whether it is registered as Singleton, Scoped or Transient.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.