Invalidate http cache when a package has been deleted from a feed #1319
Conversation
emgarten
requested review from
rohit21agrawal,
nkolev92,
zhili1208,
alpaix,
mishra14 and
jainaashish
| } | ||
|
|
||
| return await result; |
There was a problem hiding this comment.
The result is always awaited. Therefore it's not necessary to have a complicated cache data structure with AsyncLazy as a value.
There was a problem hiding this comment.
That isn't what I was seeing in the debugger. It was previously a task and getting hit multiple times. Changing to AsyncLazy fixed the issue. I don't know why a regular task would have done this, but I saw it happen several times during restore.
| /// <summary> | ||
| /// Cache on LibraryRange and TargetFramework | ||
| /// </summary> | ||
| private class DependencyInfoCacheKey : IEquatable<DependencyInfoCacheKey> |
There was a problem hiding this comment.
As an alternative, this could be just a formatted string as a key. This would eliminate the need in the separate key class.
There was a problem hiding this comment.
The class should be faster, right? This uses hashcodecombiner and equals on the same objects.
There was a problem hiding this comment.
Not necessarily faster but rather simpler. We implemented credentials cache with similar approach in CredentialService.
There was a problem hiding this comment.
I found the same cache key class existed under LibraryRangeCacheKey and switched to that, removing this private class.
I would normally agree with simplifying here, but this path in the walk is critical for perf. Every package in the unflattened graph needs to be resolved, which is much larger than the number you see in the final flattened graph in the assets file. And for each TFM/RID combination, across every project.
| // Clear the on disk and memory caches during the next request. | ||
| currentCacheContext = currentCacheContext.Clone(); | ||
| currentCacheContext.MaxAge = DateTimeOffset.UtcNow; | ||
| currentCacheContext.RefreshMemoryCache = true; |
There was a problem hiding this comment.
Consider encapsulating the above 3 lines into a factory method. This would make RefershMemoryCache truly immutable.
There was a problem hiding this comment.
Good suggestion, I'll add a helper to SourceCacheContext for this.
| catch (PackageNotFoundProtocolException ex) | ||
| { | ||
| // 2nd failure, the feed is likely corrupt or removing packages too fast to keep up with. | ||
| if (match.Provider.IsHttp && match.Provider.Source != null) |
emgarten
force-pushed
the
dev-emgarten-cacheInvalidation
branch
from
763dcbb to
f169b3f
Compare
This change adds support for invalidating the http cache on a per package id basis when a 404 is received. As part of that finding the original casing of the package has been moved into the GetDependency request where the package is downloaded to disk. Perf improvements along with this: * Packages are downloaded only if they are the best match, previously this was done a per feed basis which caused unneeded downloads * Added additional caching for nuspecs and dependency info Fixes NuGet/Home#5023 Fixes NuGet/Home#5012 Addressing feedback
emgarten
force-pushed
the
dev-emgarten-cacheInvalidation
branch
from
f169b3f to
26e8bc0
Compare
This change adds support for invalidating the http cache on a per package id basis when a 404 is received. As part of that finding the original casing of the package has been moved into the GetDependency request where the package is downloaded to disk.
Perf improvements along with this:
For NuGet.sln this improves noop restore by around 15%
The http cache size goes from 312M -> 235M due to no longer downloading extra packages.
Refactored dependency walker logic for finding the best match into ResolverUtility.cs to make it easier to test.
Cache invalidation flow:
Fixes NuGet/Home#5023
Fixes NuGet/Home#5012