Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IHttpContextAccessor cannot be used reliably in some scenarios #14975

Closed
davidfowl opened this issue Oct 14, 2019 · 13 comments · Fixed by #15049
Assignees

Comments

@davidfowl
Copy link
Member

@davidfowl davidfowl commented Oct 14, 2019

The IHttpContextAccessor is used to access the HttpContext during the current request
from components that were not given an explicit reference.

The typical use case is something like this:

public interface IUserAccessor
{
    ClaimsPrincipal User { get; }
}

public class UserAccessor : IUserAccessor
{
    private readonly IHttpContextAccessor _contextAccessor;
    public UserAccessor(IHttpContextAccessor contextAccessor)
    {
        _contextAccessor = contextAccessor;
    }

    public ClaimsPrincipal User => _contextAccessor.HttpContext?.User;
}

The IUserAccessor can now be used from components without a direct dependency on the HttpContext.

The IHttpContextAccessor can be used in 3 ways:

  1. During the http request. IHttpContextAccessor.HttpContext returns a valid value. 👍
  2. Outside of the http request scope. IHttpContextAccessor.HttpContext returns null. 👍
  3. In a context that starts on the request scope and executes beyond the scope of the request. IHttpContextAccessor.HttpContext can return null, it can throw an ObjectDisposedException on property access, or it can return the wrong value (values from the next request). This is equivalent to holding onto the HttpContext past the request lifetime. 👎

3 is a little hard to visualize so here's a coding pattern that demonstrates the issue:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

namespace WebApplication361
{
    public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddSingleton<Service>();
            services.AddHttpContextAccessor();
        }

        public void Configure(IApplicationBuilder app, IWebHostEnvironment env, Service service)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            app.UseRouting();

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapGet("/", async context =>
                {
                    context.Items["x"] = "something useful";
                    
                    await Task.WhenAny(service.SomethingSlowAsync(), Task.Delay(1000));

                    await context.Response.WriteAsync("Hello World!");
                });
            });
        }
    }

    public class Service
    {
        private readonly IHttpContextAccessor _accessor;

        public Service(IHttpContextAccessor accessor)
        {
            _accessor = accessor;
        }

        public async Task SomethingSlowAsync()
        {
            await Task.Delay(1500);

            // There's a data race here because this can be cleaned up at any time
            if (_accessor.HttpContext != null)
            {
                // Even though we did the null check above, it may have changed by the time we access the value we wanted.
                var x = _accessor.HttpContext.Items["x"];
            }
        }
    }
}

In the above example Service.SomethingSlowAsync() is performing an asynchronous operation that will take 1.5 seconds (faked with Task.Delay(1500)) then using the IHttpContextAccessor to get some information set in earlier code. The code that runs during the request waits on service.SomethingSlowAsync() for 1 second before finishing the request.

Even though the code in SomethingSlowAsync is checking that IHttpContextAccessor.HttpContext returns null, that logic ends up running in parallel with the completion of the http request. As a result, there's a data race between the null check and the code using the HttpContext.

Even though this pattern falls into the don't use the HttpContext after the request is over bucket of problems, this pattern tends to come up alot when logging. Typically, some loggers will try to use IHttpContextAccessor.HttpContext to add per request information to the logs (which is better done with logging scopes) and ApplicationInsights makes this easier with the ITelemetryInitiaizer abstraction.

Options:

  1. Do nothing. We write more docs and show why this pattern is bad and how to work around it.
  2. Lock everything all the time. Doesn't seem viable and will affect performance.
  3. Return a proxy object from the IHttpContextAccessor.HttpContext that will copy all request data just before the request ends (headers, path, method, items). Not very pay for play, needs to allocate several proxy objects per request.
@hkoelewijn

This comment has been minimized.

Copy link

@hkoelewijn hkoelewijn commented Oct 14, 2019

  1. Throw a descriptive error when HttpContext is accessed after it was disposed. Add an extension method that creates the proxy object/clone on demand, so it becomes a deliberate choice to copy the info you need before doing anything long running
@davidfowl

This comment has been minimized.

Copy link
Member Author

@davidfowl davidfowl commented Oct 14, 2019

Throw a descriptive error when HttpContext is accessed after it was disposed.

Doing this requires that we never re-use HttpContext. We can't throw this exception today because you might be using the same object instance but its being used for a different request. If the timing is right, you will get an object disposed exception.

Add an extension method that creates the proxy object/clone on demand, so it becomes a deliberate choice to copy the info you need before doing anything long running

I like that option but the responsibility is on the user to clone at the right time instead of us doing it on their behalf. This option is more pay for play but requires people to change their code.

Perhaps we could expose a method for making a safe proxy anyways and then separately decide if that's the default behavior of IHttpContextAccessor.

@mvw684

This comment has been minimized.

Copy link

@mvw684 mvw684 commented Oct 14, 2019

I would assume that with a small leigtweigth proxy in between the heavu actual requst can be reused and the cheap proxy can either have a disposed state or if requested by user contain the cloned state.
Performance and race conditions / concurency details in any aproach need to be considered

@smudge202

This comment has been minimized.

Copy link
Contributor

@smudge202 smudge202 commented Oct 14, 2019

I like the quick clone/proxy proposal - offers support to what is currently an unsupported use case.

I don't like the idea of taking a performance hit (if IHttpContextAccessor default behaviour were to change) because what is presumably a very small portion of users are incorrectly accessing the HttpContext and not using a clone/proxy workaround (if provided).

@blowdart

This comment has been minimized.

Copy link
Member

@blowdart blowdart commented Oct 14, 2019

The proxy object approach might be horrible for Windows Auth principals, and other things that use handles underneath. That'd need to be carefully tested.

@amoerie

This comment has been minimized.

Copy link

@amoerie amoerie commented Oct 14, 2019

If it were up to me, I'd look into why people are using HttpContextAccessor and not the recommended solutions instead.

It is well documented already that HttpContextAccessor is not an encouraged API, so I agree with others here that there shouldn't be a performance penalty just to accommodate a few bad actors

@davidfowl

This comment has been minimized.

Copy link
Member Author

@davidfowl davidfowl commented Oct 14, 2019

The proxy object approach might be horrible for Windows Auth principals, and other things that use handles underneath. That'd need to be carefully tested.

Yes good point! The WindowsPrincipal will be disposed and there's no saving it. The biggest problem here IMO is that there's currently no sane way to know for sure if you can access the IHttpContextAccessor if you don't know what context you are being called from.

Here's how it manifests when using ApplicationInsights:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

namespace WebApplication361
{
    public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddSingleton<ITelemetryInitializer, TelemetryInitializer>();
            services.AddSingleton<TelemetryClient>();
            services.AddSingleton<Service>();
            services.AddHttpContextAccessor();
        }

        public void Configure(IApplicationBuilder app, IWebHostEnvironment env, Service service)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            app.UseRouting();

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapGet("/", async context =>
                {
                    context.Items["x"] = "something useful";

                    await Task.WhenAny(service.SomethingSlowAsync(), Task.Delay(1000));

                    await context.Response.WriteAsync("Hello World!");
                });
            });
        }
    }

    public class Service
    {
        private readonly TelemetryClient _telemetryClient;

        public Service(TelemetryClient telemetryClient)
        {
            _telemetryClient = telemetryClient;
        }

        public async Task SomethingSlowAsync()
        {
            _telemetryClient.TrackTelemetry(new Telemetry());
            await Task.Delay(1500);
            _telemetryClient.TrackTelemetry(new Telemetry());
        }
    }

    public class TelemetryClient
    {
        private IEnumerable<ITelemetryInitializer> _telemetryInitializers;
        public TelemetryClient(IEnumerable<ITelemetryInitializer> telemetryInitializers)
        {
            _telemetryInitializers = telemetryInitializers;
        }

        public void TrackTelemetry(Telemetry telemetry)
        {
            foreach (var ti in _telemetryInitializers)
            {
                ti.IntializeTelemetry(telemetry);
            }
        }
    }

    public class TelemetryInitializer : ITelemetryInitializer
    {
        private readonly IHttpContextAccessor _accessor;

        public TelemetryInitializer(IHttpContextAccessor accessor)
        {
            _accessor = accessor;
        }

        public void IntializeTelemetry(Telemetry telemetry)
        {
            // There's a data race here because this can be cleaned up at any time
            if (_accessor.HttpContext != null)
            {
                // Even though we did the null check above, it may have changed by the time we access the value we wanted.
                telemetry.Properties["x"] = _accessor.HttpContext.Items["x"]?.ToString();
            }
        }
    }

    public interface ITelemetryInitializer
    {
        void IntializeTelemetry(Telemetry telemetry);
    }

    public class Telemetry
    {
        public Dictionary<string, string> Properties { get; set; } = new Dictionary<string, string>();
    }

    public class ExceptionTelemetry : Telemetry
    {
        public Exception Exception { get; set; }
    }
}

It's the exact same scenario I described above but way less obvious. Any call to TrackTelemetry ends up indirectly calling user code, from any context and that code has no idea whether it's safe to use the IHttpContextAccessor.

@alefranz

This comment has been minimized.

Copy link
Contributor

@alefranz alefranz commented Oct 14, 2019

I've been bitten by this as well.
My personal guidance is now to not use the IHttpContextAccessor and copy what you need from it (with the exception of some thoroughly reviewed library that needs to avoid a copy for performance reason) so I think it would make sense to improve the docs on how to work around it.
I would be even tempted to suggest to deprecate it 😅 as with the current gotchas (hard to detect) it doesn't fit with the pit of success strategy.

Is there any scenario that this enable when otherwise storing it in an async local from a middleware would be too late?
If you have to explicitly store the context yourself there is a bigger chance (am I too optimistic?) that you think about the life-cycle as well as which fields you actually need and do the right thing (and with the right trade-off).

Doing a full copy of the context, even if it is opt in, it feels like unnecessarily costly as you probably need just a small % of the data. Also, it will have to materialize all the lazy evaluated properties (e.g. headers) I imagine.

@davidfowl

This comment has been minimized.

Copy link
Member Author

@davidfowl davidfowl commented Oct 14, 2019

If you have to explicitly store the context yourself there is a bigger chance (am I too optimistic?) that you think about the life-cycle as well as which fields you actually need and do the right thing (and with the right trade-off).

I'm actually thinking that might be a better idea as well. Recommend using a custom async local that never gets cleared but is set at the beginning of every request.

@SherifRefaat

This comment has been minimized.

Copy link

@SherifRefaat SherifRefaat commented Oct 14, 2019

@davidfowl What about disabling the reuse of HttpContext when certain 'compatibility' flag is set to true? That would require no change in user's code and would be feasible to wait for the next minor release to migrate -- relatively -- older projects.

@smudge202

This comment has been minimized.

Copy link
Contributor

@smudge202 smudge202 commented Oct 14, 2019

One other consideration - IHttpContextAccessor.HttpContext smells to me a lot like DateTime.Now (sure, it's not static, but otherwise...).

If the context isn't always available, could we change that to bool IHttpContextAccessor.TryGetHttpContext(out HttpContext context)?

@davidfowl

This comment has been minimized.

Copy link
Member Author

@davidfowl davidfowl commented Oct 15, 2019

@SherifRefaat yes that's possible but it would be great to have the opt-in be more granular. This also means that you would get an object disposed exception or null. It wouldn't "fix" the problem.

@smudge202 That's not the problem. You can get the HttpContext (TryGetHttpContext would return true) and it could be cleared out from under you after that.

@davidfowl

This comment has been minimized.

Copy link
Member Author

@davidfowl davidfowl commented Oct 16, 2019

OK I have a pretty simple fix for this. PR coming soon. The result will be object disposed exceptions. That's a bit better than getting the wrong values 😄

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