Avoid NuGet restore cycles by ocallesp · Pull Request #8239 · dotnet/project-system

ocallesp · 2022-06-11T01:13:44Z

Avoid infinite NuGet restores.
Scenario:
Projects might have a package that sets AssetTargetsFallback value that can cause a certain PackageReference to be included. This package contains a props file that changed the AssetTargetFallback property value, and the PackageReference Condition was no longer true, causing it to be removed. Once removed, the AssetTargetFallback value from the csproj was used again, causing an infinite restore -> design time build -> restore loop.

This change:
Since DotNet ProjectSystem can detect changes in NuGet restores by calculating a hash, we can use this hash to detect changes in AssetTargetFallBack and check when this value gets repeated.

This pr makes use of the NuGetRestoreCycleDetector to keep the last N hashes, and once it detects a repeated value (cycle) Dotnet ProjectSystem can stop NuGet restores.

Microsoft Reviewers: Open in CodeFlow

nkolev92 · 2022-07-15T02:05:08Z

Any updates on this change @ocallesp?

MiYanni · 2022-08-05T01:17:09Z

@ocalles You're getting build failures with these changes.

(CoreCompile target) -> 
         D:\a\_work\1\s\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\ProjectSystem\VS\PackageRestore\PackageRestoreDataSource.cs(7,44): error CS0234: The type or namespace name 'Implementation' does not exist in the namespace 'Microsoft.VisualStudio.ProjectSystem' (are you missing an assembly reference?)
         D:\a\_work\1\s\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\ProjectSystem\VS\PackageRestore\PackageRestoreDataSource.cs(55,26): error CS0246: The type or namespace name 'IInfoBarService' could not be found (are you missing a using directive or an assembly reference?)
         D:\a\_work\1\s\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\ProjectSystem\VS\PackageRestore\PackageRestoreDataSource.cs(68,13): error CS0246: The type or namespace name 'IInfoBarService' could not be found (are you missing a using directive or an assembly reference?)

ocallesp · 2022-08-08T17:45:34Z

@ocalles You're getting build failures with these changes.

(CoreCompile target) -> 
         D:\a\_work\1\s\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\ProjectSystem\VS\PackageRestore\PackageRestoreDataSource.cs(7,44): error CS0234: The type or namespace name 'Implementation' does not exist in the namespace 'Microsoft.VisualStudio.ProjectSystem' (are you missing an assembly reference?)
         D:\a\_work\1\s\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\ProjectSystem\VS\PackageRestore\PackageRestoreDataSource.cs(55,26): error CS0246: The type or namespace name 'IInfoBarService' could not be found (are you missing a using directive or an assembly reference?)
         D:\a\_work\1\s\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\ProjectSystem\VS\PackageRestore\PackageRestoreDataSource.cs(68,13): error CS0246: The type or namespace name 'IInfoBarService' could not be found (are you missing a using directive or an assembly reference?)

Fixed.
I added the implementation of the InfoBar service as part of this pr.

ocallesp · 2022-08-08T18:16:50Z

Tests:
Open a solution having an invalid NuGet package. Cycle was detected.
Open roslyn and orchardcore. No cycles detected.

ocallesp · 2022-08-08T18:21:54Z

Any updates on this change @ocallesp?

Today I add a new commit with the implementation of InfoBar. This pr is complete. Waiting for reviewers and approval

kvenkatrajan · 2022-08-08T18:27:11Z

@ocallesp - not required for the initial merge but anyways we can add telemetry around this for when we detect a cyclic restore? Will help determine how often our users run into this scenario, we detect it & display the infobar

jacdavis · 2022-08-08T22:16:22Z

cycledetected

Don't forget to get this blessed once it starts coming through.

think it would be helpful to add additional properties in addition to cycledetected. So have the event name as :
/nugetrestore/cycledetection and properties like vs.projectsystem.managed.nugetrestore.cycledetection.cycledetected, vs.projectsystem.managed.nugetrestore.cycledetection.durationmillis, vs.projectsystem.managed.nugetrestore.cycledetection.successfulrestorecount,
vs.projectsystem.managed.nugetrestore.cycledetection.projectid or vs.projectsystem.managed.nugetrestore.cycledetection.solutionid perhaps. Any additional items you think may be useful to help troubleshoot when we detect a cyclical restore.

I believe project id and solutionid are sent automatically. Duration and succesffulrestorecount would be nice. I think succesffulrestorecount will be unreliable as I believe we must stop as soon as a cycle is detected.

jacdavis · 2022-08-08T22:19:40Z

if (ErrorHandler.Succeeded(vsShell.GetProperty((int)__VSSPROPID.VSSPROPID_IsInCommandLineMode, out object result)) && (bool)result)

Are we sure this always executes on the ui thread? IIRC, IVsShell requires the ui thread and doesn't have a proxy.

jacdavis · 2022-08-08T22:20:15Z

GetProperty

Same as above. What thread does this execute on? I would feel better about a JTF call to ensure we're on the ui thread.

jacdavis · 2022-08-08T22:31:59Z

if (!_vsShellServices.IsInServerMode)

What is the right behavior if this is in server mode? Right now, it silently succeeds.

Are you sure IsInServerMode works off the ui thread? This is invoked on a arbitrary thread in cycle detector.

IsInServerMode -> is this to account for Nexus based scenarios? Do we need to explicitly call out a NotSupportedException when IsInServerMode = true?

This was originally to account for Nexus scenarios, yes, and as we don't really have a server SKU under active development it probably doesn't matter much. I think it's reasonable to throw an exception if we are in server mode, and also agree with @jacdavis that we need to verify IsInServerMode is ok to call from an arbitrary thread, and/or transition to the UI thread.

jacdavis · 2022-08-08T22:46:20Z

Queue

Why are we using a queue here and not some data structure with O(1) lookup? I expected this to be a hash table of hashes. A queue doesn't make a lot of sense in my mind. Am I missing some info? #Closed

I added the hash _lookupTable to improve the search to O(1)

I'm still not convinced a queue is the correct data structure here. Can you please call me to discuss as requested?

tmeschter · 2022-08-10T23:12:28Z

This was originally to account for Nexus scenarios, yes, and as we don't really have a server SKU under active development it probably doesn't matter much. I think it's reasonable to throw an exception if we are in server mode, and also agree with @jacdavis that we need to verify IsInServerMode is ok to call from an arbitrary thread, and/or transition to the UI thread.

This a mef exported service that displays an infoBar in the main windows.

The cycle detector is based on hashes calculated from restore inputs. This algorithm detects patterns A -> B -> A Time O(1) search Time O(1) add Time O(1) delete Space O(1)

ocallesp · 2022-08-13T01:04:47Z

Some of the pending code review suggestions will be fixed as part of #8389

Fixes dotnet#8504 This interface and its implementation (`VSShellServices`) was copied from CPS in dotnet#8239, but nothing ever initialised the implementation. This change changes the API so that these values can be lazily implemented and efficiently queried.

ocallesp requested a review from drewnoakes June 11, 2022 01:32

ocallesp changed the title ~~Avoid nu get restore cycles~~ Avoid NuGet restore cycles Jun 11, 2022

drewnoakes reviewed Jun 11, 2022

View reviewed changes

Comment thread ...Studio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/NuGetRestoreCycleDetector.cs Outdated

ocallesp requested a review from nkolev92 June 13, 2022 18:49

drewnoakes reviewed Jun 14, 2022

View reviewed changes

Comment thread ...lStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/PackageRestoreDataSource.cs Outdated

drewnoakes reviewed Jun 14, 2022

View reviewed changes

Comment thread ...lStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/PackageRestoreDataSource.cs Outdated

drewnoakes reviewed Jun 14, 2022

View reviewed changes

Comment thread ...Studio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/NuGetRestoreCycleDetector.cs Outdated

ocallesp mentioned this pull request Jul 15, 2022

VS does not stop restoring #8326

Closed

ocallesp marked this pull request as ready for review August 4, 2022 20:33

ocallesp requested a review from a team as a code owner August 4, 2022 20:33

ocallesp requested review from jacdavis and tmeschter August 8, 2022 17:43

ocallesp mentioned this pull request Aug 8, 2022

Add Telemetry properties to the NuGet cycle detector #8389

Closed

jacdavis reviewed Aug 8, 2022

View reviewed changes

Comment thread ...ProjectSystem.Managed.VS/ProjectSystem/VS/UI/InfoBarService/VsInfoBarService.InfoBarEntry.cs Outdated

jacdavis reviewed Aug 8, 2022

View reviewed changes

Comment thread ...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/UI/InfoBarService/VsInfoBarService.cs Outdated

jacdavis reviewed Aug 8, 2022

View reviewed changes

Comment thread ...lStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/PackageRestoreDataSource.cs Outdated

jacdavis reviewed Aug 8, 2022

View reviewed changes

Comment thread ...Studio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/NuGetRestoreCycleDetector.cs Outdated

jacdavis reviewed Aug 8, 2022

View reviewed changes

nkolev92 reviewed Aug 10, 2022

View reviewed changes

Comment thread ...lStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/PackageRestoreDataSource.cs Outdated

ocallesp commented Aug 10, 2022

View reviewed changes

Comment thread ...lStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/PackageRestore/PackageRestoreDataSource.cs Outdated

tmeschter approved these changes Aug 10, 2022

View reviewed changes

ghost added Feature-NuGet NuGet integration including pushing it properties, project and package references, and Pack support. Performance-Scenario-General This issue affects performance in general. Tenet-Performance This issue affects the "Performance" tenet. labels Aug 11, 2022

ocalles added 4 commits August 12, 2022 14:13

Implement an InfoBar Service

dbb4f8b

This a mef exported service that displays an infoBar in the main windows.

Implement an algorithm to detect NuGet restore cycles

8770159

The cycle detector is based on hashes calculated from restore inputs. This algorithm detects patterns A -> B -> A Time O(1) search Time O(1) add Time O(1) delete Space O(1)

Add localized text message for the NuGet restore cycle infoBar

5b53218

Check for NuGet restore cycle

17b5982

ocallesp merged commit 6618ea8 into dotnet:main Aug 12, 2022

ghost added this to the 17.4 milestone Aug 12, 2022

nkolev92 reviewed Aug 22, 2022

View reviewed changes

Comment thread src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/VSResources.resx

drewnoakes mentioned this pull request Sep 19, 2022

VSShellServices never initialised #8504

Closed

This was referenced Sep 19, 2022

Language service runs when populating solution cache #8506

Merged

Remove mention of NU1108 from message #8507

Closed

Remove NU1108 from message in NuGet restore cycles detection #8509

Merged

ocallesp deleted the AvoidNuGetRestoreCycles branch September 21, 2022 21:02

This was referenced May 30, 2023

NuGet restore cycle detection doesn't work #9050

Closed

Fix package restore cycle detection #9051

Merged

Conversation

ocallesp commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkolev92 commented Jul 15, 2022

Uh oh!

MiYanni commented Aug 5, 2022

Uh oh!

ocallesp commented Aug 8, 2022

Uh oh!

ocallesp commented Aug 8, 2022

Uh oh!

ocallesp commented Aug 8, 2022

Uh oh!

kvenkatrajan commented Aug 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacdavis Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocallesp commented Aug 13, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ocallesp commented Jun 11, 2022 •

edited

Loading

jacdavis Aug 8, 2022 •

edited

Loading