Adding Cache.Insert overload with 3 arguments #350
Conversation
|
Not entirely sure why this build is failing (Verify TypeForwards/Reference assembly) https://dev.azure.com/dnceng-public/public/_build/results?buildId=292536&view=logs&jobId=1f58c02c-a2e7-59c3-5915-ab7de64e5b92&j=1f58c02c-a2e7-59c3-5915-ab7de64e5b92&t=866826a7-f347-5daa-9ba3-e9164fa7bbbe |
|
You'll want to run |
|
|
||
| public void Insert(string key, object value, CacheDependency? dependencies) | ||
| { | ||
| var policy = new CacheItemPolicy |
There was a problem hiding this comment.
What about the dependency argument? The original implementation is here: https://referencesource.microsoft.com/#System.Web/Cache/cache.cs,d083199dcfb5f0da - feel free to copy the implementation from there to here.
There was a problem hiding this comment.
Oh right yeah, sorry the signature is not correct. Will fix later today.
There was a problem hiding this comment.
Is that why the build is failing btw?
There was a problem hiding this comment.
@twsouthwick CacheDependency argument is not currently being consumed in other methods but there is an unused private AddChangeMonitors that was introduced in this commit 66276e5#diff-bfdc712f9e9845bfd447d4eb1cbbafdb48e1d076e57108d882eb3651699290a9R52 -- This is a good potential solution to consume and monitor the cache dependencies.
I'm happy to refactor other methods as well but I wanted to double check with you first.
There was a problem hiding this comment.
@CZEMacLeod you did the original implementation here - thoughts?
There was a problem hiding this comment.
@twsouthwick I know there was a little bit of code missing from the implementation that I found when I was doing the VirtualPathProvider implementation.
There are changes in my branch https://github.com/CZEMacLeod/systemweb-adapters/tree/czemacleod/vpp-1.1 that I think include this overload and also sort out some other stuff that was not fully implemented.
I've been meaning to complete the branch for VPP and submit a PR, but while I have a good test/sample implementation, I don't have any good explicit tests defined.
It would probably be good if I separated out all the Cache changes from the VPP changes, as they are fixes/enhancements rather than a new feature but I haven't had time to complete this work yet.
Let me see if I can update my branch and pull all the cache specific code out into a PR.
One thing I found was that there were a few things (like HasChanged) that don't behave quite as logically as you would expect on the System.Web Cache implementation compared to the more generic version that was introduced later. I know that I did catch these inconsistencies and match them like for like in the update I did.
There was a problem hiding this comment.
I created #351 which contains all the changes I had queued for Cache and CacheDependency.
I've added the tests from this PR, and the missing overload for Cache.Insert. The test here for NoSliding/NoAbsolute is technically covered by Cache.Insert(String, Object)
I also have tests when there is an actual dependent object (file or item).
| HasChanged = changeMonitors.Any(cm => cm.HasChanged && (cm.GetLastModifiedUtc() > utcStart)); | ||
| utcLastModified = changeMonitors.Max(cm => cm.GetLastModifiedUtc()); | ||
| if (HasChanged) | ||
| if (changeMonitors.Count > 0) |
There was a problem hiding this comment.
This check is because changeMonitors count can be 0 (See Init())
There was a problem hiding this comment.
wouldn't this not reset HasChanged if it has potentially been set to something? Is this a correctness issue?
There was a problem hiding this comment.
changeMonitors.Any works and sets HasChanged if changeMonitors count is 0. However, utcLastModified might have an issue if the source list is empty. However, I think that the specific overload of Max for DateTime uses the generic version which uses default(T) in the case of the empty set rather than throwing an error. In this case, it should probably use the test, and set DateTime.MinValue otherwise. I think all the other paths just short circuit (e.g. the Foreach) when there are no items so I don't believe it causes any issue.
|
@twsouthwick sounds good. thanks. |
3 participants
PR Title
Adds missing Cache.Insert overload
PR Description
Caching.Cache.Insert is missing the overload with 3 arguments, see: https://learn.microsoft.com/en-us/dotnet/api/system.web.caching.cache.insert?view=netframework-4.8.1#system-web-caching-cache-insert(system-string-system-object-system-web-caching-cachedependency)
Addresses #347