Skip to content

Conversation

@CZEMacLeod
Copy link
Contributor

@CZEMacLeod CZEMacLeod commented Feb 3, 2023

Full support for CacheDependency and accessors.
Complete the implementation of System.Web.Caching.CacheDependency and provide all the accessors from System.Web implementation.

Description
Implement all constructors and features of System.Web.Caching.CacheDependency
Add extensions to allow handling various underlying ChangeMonitor implementations to surface the UtcLastModified property.
Implements CacheDependencyChangeMonitor to allow a CacheDependency to rely on another item in the Cache.
Adds Cache accessor to HttpContext, HttpContextBase, HttpContextWrapper, and HttpRuntime
Modifies HttpRuntimeFactory to allow getting the Cache implementation
Updates tests to support new IHttpRuntime interface member.

Addresses #26

Move caching to HttpRuntime and make all Cache accessor methods use that entry point
Add Cache accessors to HttpRuntime and HttpContextBase
@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2023

Test failure:

    Microsoft.AspNetCore.SystemWebAdapters.HttpContextTests.CacheFromServices [FAIL]
      System.InvalidOperationException : HttpRuntime is not available in the current environment
      Stack Trace:
        /_/src/Microsoft.AspNetCore.SystemWebAdapters/HttpRuntime.cs(14,0): at System.Web.HttpRuntime.get_Current()
        /_/src/Microsoft.AspNetCore.SystemWebAdapters/HttpRuntime.cs(25,0): at System.Web.HttpRuntime.get_Cache()
        /_/src/Microsoft.AspNetCore.SystemWebAdapters/HttpContext.cs(45,0): at System.Web.HttpContext.get_Cache()
        /_/test/Microsoft.AspNetCore.SystemWebAdapters.Tests/HttpContextTests.cs(152,0): at Microsoft.AspNetCore.SystemWebAdapters.HttpContextTests.CacheFromServices()

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the thread safety model here? I'm a little surprised not to see any locking anywhere.

@CZEMacLeod
Copy link
Contributor Author

What's the thread safety model here? I'm a little surprised not to see any locking anywhere.

The Cache implementation is a wrapper around a System.Runtime.Caching.ObjectCache implementation (System.Runtime.Caching.MemoryCache by default) and all the locking etc. logic would be implemented there.

I don't think any of the code I've added actually requires any specific locking logic, although maybe I've missed a scenario where it would be applicable?

Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! My main concern is around the use of the static HttpRuntime. I'm fine exposing APIs that were there in ASP.NET Framework, but internally, we shouldn't be using those statics for things but access it through the DI container.

@CZEMacLeod
Copy link
Contributor Author

CZEMacLeod commented Feb 10, 2023

We shouldn't be using those statics for things but access it through the DI container.

It think it was a combination of copying from the original design in net4 and thoughts to performance, as if we hit DI every time we need access to the cache, rather than using a static property that is initialized once from the singleton, it might hurt the performance.
This would happen e.g. when trying to get an item from the cache, not finding it, then adding it.
https://learn.microsoft.com/en-us/previous-versions/aspnet/xhy3h9f9(v=vs.100)

string cachedString;
cachedString = (string)HttpRuntime.Cache["CacheItem"];
if (cachedString == null)
{
  cachedString = "Hello, World.";
  HttpRuntime.Cache.Insert("CacheItem", cachedString);
}

Where HttpRuntime could also be HttpContext/HttpContextBase.

I have removed this from BaseHttpRuntime, and now all the accessor methods always hit DI. I guess if it is an issue, then the cache can be stored in a local variable in code. 685ce46#r100106939

@twsouthwick twsouthwick changed the title Full support for CacheDependency and accessors. Closes #26 Full support for CacheDependency and accessors Feb 10, 2023
@twsouthwick twsouthwick merged commit 66276e5 into dotnet:main Feb 11, 2023
@CZEMacLeod CZEMacLeod deleted the czemacleod/cache branch June 6, 2023 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants