Skip to content
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

Merged

Conversation

zivkan
Copy link
Member

@zivkan zivkan commented Aug 30, 2023

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:

// For installs only use cache entries newer than the current time.
// This is needed for scenarios where a new package shows up in search
// but a previous cache entry does not yet have it.
// So we want to capture the time once here, then pass it down to the two
// restores happening in this flow.
var now = DateTimeOffset.UtcNow;
void cacheModifier(SourceCacheContext cache) => cache.MaxAge = now;

(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:

  • Keep the comments about the enum helpers in AuditUtils, because they still apply
  • Add a big comment in VulnerabilityInformationProvider.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

    • Automated tests added
    • OR
    • Test exception: reverting a commit that didn't have tests. Under time pressure to meet shipping dates, but we can consider some kind of complicated functional test to validate this scenario in the future.
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

Sorry, something went wrong.

@zivkan zivkan requested a review from a team as a code owner August 30, 2023 09:41
@@ -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));
Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member Author

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 SourceCacheContexts, 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.

Copy link
Member

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));
Copy link
Contributor

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

@zivkan zivkan merged commit f5a29ef into dev Sep 5, 2023
@zivkan zivkan deleted the dev-zivkan-revert-vulnerability-data-cache-context-reuse branch September 5, 2023 10:43
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.

None yet

3 participants