alastairtree / LazyCache

An easy to use thread safe in-memory caching service with a simple developer friendly API for c#
https://nuget.org/packages/LazyCache
MIT License
1.72k stars 159 forks source link

HttpContext is lost when executing async methods #110

Open ArnaudB88 opened 4 years ago

ArnaudB88 commented 4 years ago

I want to cache the result of a method with LazyCache. That method uses other classes which are instantiated with dependency injection. Instances of that class are created with a lifetime manager using the HTTPContext. In short, the 'addItemFactory' method which I want to cache the result of, needs the HttpContext.

var result = appCache.GetOrAddAsync("foo", async () => {
await Task.Delay(1000);
//Here a method is executed which need the HttpContext, can be seen as:
return System.Web.HttpContext.Current.Request;
}

It seems that when the execution is started of the async method, the HTTP context is lost. The point where it loses the context is the contructor:

LazyCache.AsyncLazy<T>.AsyncLasy(taskFactory) :
base(() => Task.Factory.StartNew(taskFactory).Unwrap())

When starting a new task (Factory.StartNew()), the context is lost.

ArnaudB88 commented 4 years ago

My pull request was closed for the reason that the library should not depend on System.Web and because the solution was not 100% safe. https://github.com/alastairtree/LazyCache/pull/111#issuecomment-607703593

Following suggested solution does not work:

var httpContext = System.Web.HttpContext.Current;
var addItemFactory = async entry => 
{
    // here you can use httpContext instead of System.Web.HttpContext.Current
};
await AppCache.GetOrAddAsync(key, addItemFactory, absoluteExpiration);

Error: 'The name 'httpContext' does not exist in the current context'.

Possible next steps:

ArnaudB88 commented 4 years ago

@alastairtree Can you please take a look at my suggestions above? Thanks!

alastairtree commented 4 years ago

Never use HttpContext object outside the context of the current http request - this is a general rule for asp.net to avoid memory leaks/odd behaviours, not just a lazy cache thing. You need to pass the values not the context into the delegate to avoid captring the context, e.g if i need the url and content type from the context i could pass them like this:

var url = System.Web.HttpContext.Current.Request.Url.ToString();
var contentType = System.Web.HttpContext.Current.Request.Headers["content-type"].Value;

var addItemFactory = async entry => 
{
    // here you CANNOT use httpContext, but you can use the url and contentType
};
await AppCache.GetOrAddAsync(key, addItemFactory, absoluteExpiration);
ArnaudB88 commented 4 years ago

As you can see in my original post, that is not an option:

That method uses other classes which are instantiated with dependency injection. Instances of that class are created with a lifetime manager using the HTTPContext.

More specific, some database data must be cached and for instantiating the EntityFramework dbContext with dependency injection, the httpContext is needed.

I know using the HttpContext outside the context of the http request is not a good solution, therefor my 3 suggestions above. With the first two suggestions, caching remains in the same context.

alastairtree commented 4 years ago

Without a fully detailed example it is hard to solve your particluar issue, but as long as the method whose results you want to cache first runs in the context of the request then lazy cache will only cache the result of the method and the http context will not be needed outsode request lifetime. Maybe the issue is that you are not awaiting the cached value, as in your example you are missing the await:

// remeber to await the result
var result = await appCache.GetOrAddAsync("foo", async () => {
    //call some method that needs to run in request lifetime with lifetime dependent on htt context
   var SomeObjectNotReferencingHttpContext  = await thingDependingOnHttpContext.GetStuff();

   return SomeObjectNotReferencingHttpContext;
}
ArnaudB88 commented 4 years ago

I added a unit test project illustrating the behavior I am trying to explain: https://github.com/ArnaudB88/LazyCache/tree/Bug_110

See the project LazyCache.UnittestsAspNet. The cached method should be executed in the current HttpContext since it's a database call, and database connections are set to 'one per http call' (=PerRequest lifetime manager in dependency injection)

alastairtree commented 4 years ago

I have looked at your test code from here.

You cannot do dependency injection resolution (_container.Resolve<IDbContext>();) inside the cached function LongRunningDatabaseCallAsync because that func does not run on the http request thread and so does not have access to the ambient http context. The only way of solving I can think of is to split up the resolution and the database call, and only cache the latter, something like so, using your test code:

            ....
            //3. Retrieve cached value needing httpContext
            var appCache = _container.Resolve<IAppCache>();
            var dbContext = _container.Resolve<IDbContext>();
            try
            {
                var cachedValue = await appCache.GetOrAddAsync("foo",
                    async () => await dbContext.GetInfoFromDatabaseAsync());

                Assert.AreEqual(DbContext.EXPECTED_VALUE, cachedValue);
            }
            catch(Exception e)
            {
                Assert.Fail(e.Message);
            }

In summary - you must refactor, the cached func will never have access to the http context, you must pass in values or references from the context you require into the cached func.

ArnaudB88 commented 4 years ago

Thank you for looking at my test code. Your suggested solution indeed works for simple DB calls and is a possible workaround. But when trying to cache a method which itself calls different methods (and so-on), it is impossible to do all the DI resolving before the caching starts.

I still don't think that a caching operation needs to start a new thread, and so losing it's context. I am curious about the official MS implementation (dotnet/runtime#27510). Maybe that can be used if available in the future.

alastairtree commented 4 years ago

It's not LazyCache starts a new thread, it is that because it is async the resumed task will be picked up by an availed thread from the pool you don't get any guarantees that the thread that resumes is the same one that initiated it. This is by design in the framework in the general sense for optimal performance and to minimise deadlocks.

The changing context is a result of using Lazy - the context that creates the lazy, is not necessarily the same context that will call .Result. this is a very deliberate design decision to achieve the required performance and locking.

Because you are caching, the cached result probably needs to be abstracted away from the http request and context anyway. E.g. two users might share the same cached list of products in an e-commerce website, so the product fetching code should not reference the user, otherwise the second user might then execute code in the context of the first user.

As general guidance, it is useful to have clear separation of concerns between web stuff associated with the http request such as headers, auth and references to the HttpContext and then back end business logic and data access.

If this absence of context is causing you issues and you cannot refactor around it, probably LazyCache is not a great fit for you, and you might be better off using MemoryCache directly, or just a static dictionary.

alastairtree commented 3 years ago

Mind if I close this issue now? Or still need anything?

ArnaudB88 commented 3 years ago

Because you are caching, the cached result probably needs to be abstracted away from the http request and context anyway

My web application needs a httpContext when performing database calls because the dbContext is scoped at 'http request' level. This is exactly for separation of user/http calls to the db. In my situation, only the first http call would need to go to the database because the data won't change in a day.

I still hope some changes will be done to allow running delegates within the initial context.

An interesting comment is posted on the dotnet AsyncLazy issue: https://github.com/dotnet/runtime/issues/27510#issuecomment-711037637 In particular point '2' in his comment where he gives his advise when he would implement AsyncLazy from a clean page.

He says that he would implement it running on the current context by default. In his own library he gave the choice with a flag because of backwards compatibility reasons.

Steve887 commented 2 years ago

@ArnaudB88 did you ever resolve this? I ran into the same issue today needing the HttpContext inside my cache object. Tried a bunch of options with no effect.

ArnaudB88 commented 2 years ago

@Steve887 This is still not fixed in LazyCache. Currently you have 2 options: