-
Notifications
You must be signed in to change notification settings - Fork 701
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
Revert "Pass through the cache context to the vulnerability resource (#5361)" #5387
Revert "Pass through the cache context to the vulnerability resource (#5361)" #5387
Conversation
@@ -120,7 +120,7 @@ public class RestoreCommandProvidersCache | |||
var vulnerabilityInformationProviders = new List<IVulnerabilityInformationProvider>(sources.Count); | |||
foreach (SourceRepository source in sources) | |||
{ | |||
IVulnerabilityInformationProvider provider = _vulnerabilityInformationProviders.GetOrAdd(source, s => new VulnerabilityInformationProvider(s, cacheContext, log)); | |||
IVulnerabilityInformationProvider provider = _vulnerabilityInformationProviders.GetOrAdd(source, s => new VulnerabilityInformationProvider(s, log)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only true problem correct? Because the SourceCacheContext here is the one passed down from "up above" which can be the NuGet Package Manager.
IMO, we should still create a sourcecachecontext here, and not in the VulnerabilityInformationProvider.
That way this change is basially a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 sounds fine to me, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting back to the dev branch and and having RestoreCommandProvidersCache
create its own SourceCacheContext
caused CA2000. But this code only creates an instance of the VulnerabilityInformationProvider
, which means it's not appropriate to dispose the SourceCacheContext
when RestoreCommandProvidersCache.GetOrAdd
returns. This class is part of our public APIs, so there's no good solution.
I opted to make the SourceCacheContext
a field, but then I had to make the class implement IDisposable
to avoid CA1001. Now CA2000 and CA1001 triggered everywhere that creates RestoreCommandProvidersCache
instances. It wasn't too bad, until NuGet.PackageManagement.NuGetPackageManager
has a field holding an instance, so now it "needs" to become IDisposable
as well. Looking at references to NuGetPackageManager
constructors, there are 18 calls, although 3 are probably just from other overloads. This is where I decided this path was too much work.
Another idea I had to is change RestoreCommandProvidersCache.GetOrAdd
to pass in 2 SourceCacheContext
s, one just for vulnerabilities. However, looking at how the current cache context is passed in, it's coming from RestoreArgs.CacheContext
. So do I need to add VulnerabilityCacheContext
to RestoreArgs
? That didn't seem like a good option.
My last idea is to add more properties to SourceCacheContext
. I didn't investigate how much needs to change to make this works, but I expect it'll be a really bad change because ultimately HttpSourceCacheContext
would need a bool
or something to tell it to either use the existing properties, or to use the new (vulnerability only?) properties, almost all of which are public APIs.
In conclusion, I don't see a way to implement your proposal in a low-impact way (well, unless we don't dispose a disposable object, and let the GC dispose it, and suppress CA2000 in RestoreCommandProvidersCache
).
If you, or someone else, wants to reduce memory allocations by reducing new instances of SourceCacheContext
, then they can spend the effort solve the above problems. We're only creating one instance per source, not per project, and downloading over HTTP the vulnerability database has a higher customer impact than any potential GC from extra allocations of the SourceCacheContext
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for trying
@@ -120,7 +120,7 @@ public class RestoreCommandProvidersCache | |||
var vulnerabilityInformationProviders = new List<IVulnerabilityInformationProvider>(sources.Count); | |||
foreach (SourceRepository source in sources) | |||
{ | |||
IVulnerabilityInformationProvider provider = _vulnerabilityInformationProviders.GetOrAdd(source, s => new VulnerabilityInformationProvider(s, cacheContext, log)); | |||
IVulnerabilityInformationProvider provider = _vulnerabilityInformationProviders.GetOrAdd(source, s => new VulnerabilityInformationProvider(s, log)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 sounds fine to me, too
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/2461
Regression? yes Last working version: before 9ea2def
Description
Preview install (which happens both for PM UI install, but also when other VS components call our IVsPackageInstaller.InstallPackage API), ignores the http-cache, so that newly published packages do not fail to install:
NuGet.Client/src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs
Lines 2935 to 2941 in e4a9922
(also see other references to
SourceCacheContext.MaxAge
)However, that "newly published package" scenario does not apply to the vulnerability database. There's no reason to force customers to redownload the vulnerability database, so this reverts the previous commit.
This PR isn't a straight revert, it has two modifications:
AuditUtils
, because they still applyVulnerabilityInformationProvider.cs
explaining why we're using a different cache context.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation