Added check for circular dependencies #3474

etbyrd · 2018-04-18T03:48:06Z

Customer scenario

In certain circumstances, there can be a circular dependency that creates a stack overflow when recursively checking for unresolved child dependencies, causing a complete VS crash.

Bugs this fixes:

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/487858
#3374

Workarounds, if any

None

Risk

Low, there are some threading concerns that I am looking into to make sure that the Hashset is not shared - but I do not think that will be an issue because I am editing a private method and the class is instantiated when creating a snapshot.

Performance impact

Low. Just adding a HashSet.

Is this a regression from a previous update?

No

Root cause analysis:

There was no check that a dependency was not already resolved.

How was the bug found?

Customer feedback.

codecov-io · 2018-04-18T03:58:49Z

Codecov Report

Merging #3474 into dev15.7.x will increase coverage by 0.13%.
The diff coverage is 93.87%.

@@              Coverage Diff              @@
##           dev15.7.x    #3474      +/-   ##
=============================================
+ Coverage      68.29%   68.43%   +0.13%     
=============================================
  Files            731      731              
  Lines          36675    36719      +44     
  Branches        2119     2120       +1     
=============================================
+ Hits           25049    25128      +79     
+ Misses         11215    11172      -43     
- Partials         411      419       +8

Flag	Coverage Δ
#debug	`68.43% <93.87%> (+0.13%)`	⬆️

Impacted Files	Coverage Δ
...cies/Snapshot/TargetedDependenciesSnapshotTests.cs	`97.82% <100%> (+0.12%)`	⬆️
...endencies/Snapshot/TargetedDependenciesSnapshot.cs	`76.92% <84.21%> (+19.7%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93f5e53...52fa309. Read the comment docs.

jmarolf · 2018-04-18T05:06:42Z

@etbyrd is there a way to write a test for this case?

abpiskunov · 2018-04-18T16:11:08Z

...ystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshot.cs

@@ -55,6 +55,8 @@ internal class TargetedDependenciesSnapshot : ITargetedDependenciesSnapshot
        private Dictionary<string, bool> _unresolvedDescendantsMap
            = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase);

+        private HashSet<string> _seenDependencies = new HashSet<string>();


private HashSet _seenDependencies = new HashSet(); [](start = 7, length = 67)

Why keep it global? It seems like implementation detail of FindUnresolvedDependenciesRecursive and don't have to stay in memory? We already keeping too much stuff in memory ...

Good point. I will fix that

abpiskunov · 2018-04-18T16:19:04Z

    public bool CheckForUnresolvedDependencies(IDependency dependency)

I know that tests for this method are missing here https://github.com/dotnet/project-system/blob/master/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshotTests.cs but i would expect them to be added with next changes and now looks like a good moment :)

Refers to: src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshot.cs:78 in 6a424b2. [](commit_id = 6a424b2, deletion_comment = False)

etbyrd · 2018-04-18T17:48:07Z

@jmarolf @abpiskunov I will be adding tests for this - I am still worried about some edge cases

Pilchie · 2018-04-19T20:11:46Z

...ystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshot.cs

@@ -55,6 +55,8 @@ internal class TargetedDependenciesSnapshot : ITargetedDependenciesSnapshot
        private Dictionary<string, bool> _unresolvedDescendantsMap
            = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase);

+
+


Nit: remove this added blank space.

abpiskunov

jmarolf · 2018-04-20T22:59:51Z

...S.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshotTests.cs

@@ -573,6 +573,47 @@ public void TFromChanges_RemovedAndAddedChanges()
            Assert.True(snapshot.DependenciesWorld.ContainsKey(@"tfm1\xxx\addeddependency3"));
        }

+        /// <summary>
+        /// Added because circular dependencies can cause stack overflows
+        /// https://github.com/dotnet/project-system/pull/3474


Link to #3374 instead please

Pilchie · 2018-04-20T23:27:56Z

...ystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshot.cs

+        /// <summary>
+        /// Internal for testing.
+        /// </summary>
+        internal TargetedDependenciesSnapshot(IDictionary<string, IDependency> dependenciesWorld, IEnumerable<IDependency> topLevelDependencies)


I would rather make this constructor private, with in internal static factory method with a clear name like CreateForUnitTestingOnly or something.

Pilchie · 2018-04-20T23:30:34Z

...ystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshot.cs

+                    //Checking here will prevent a stack overflow due to rechecking the same dependencies
+                    if (_dependenciesChildrenMap.ContainsKey(child.Id))
+                    {
+                        unresolved = false;


I had to think about why this is sufficient to break cycles. My conclusion is that we don't have to worry about the unresolved case, because unresolved dependencies wouldn't have any children, is that right?

Pilchie · 2018-04-20T23:30:48Z

...ystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshot.cs

+                        break;
+                    }
+
+                    //If the dependency is already in the child map, it is resolved


Nit: We put a space after // in comments.

Pilchie · 2018-04-20T23:31:14Z

...S.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshotTests.cs

+                topLevelDependencies: new List<IDependency>() { dependencyModelTop1 });
+
+            // verify it doesn't stack overflow
+            previousSnapshot.CheckForUnresolvedDependencies(dependencyModelTop1);   


Have you verified that this test fails without your fix?

Yes, it fails without the fix.

jmarolf · 2018-04-21T03:12:10Z

...S.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/TargetedDependenciesSnapshotTests.cs

+                topLevelDependencies: new List<IDependency>() { dependencyModelTop1 });
+
+            // verify it doesn't stack overflow
+            previousSnapshot.CheckForUnresolvedDependencies(dependencyModelTop1);   


I would prefer if we didn't have the failure case here be: crash the test harness. That is going to be hard to diagnose if we ever regress this.

I agree. I am working a way around this but I wonder if it is a requirement for this fix to be added.

etbyrd changed the title ~~added check for circular dependencies~~ [WIP] added check for circular dependencies Apr 18, 2018

etbyrd requested review from davkean and a team April 18, 2018 03:49

abpiskunov reviewed Apr 18, 2018

View reviewed changes

Pilchie approved these changes Apr 19, 2018

View reviewed changes

etbyrd force-pushed the circular-dependency-check branch 2 times, most recently from b24daa6 to 6a424b2 Compare April 19, 2018 20:18

abpiskunov approved these changes Apr 19, 2018

View reviewed changes

etbyrd force-pushed the circular-dependency-check branch from 89be799 to 9ccfcbc Compare April 20, 2018 22:25

etbyrd changed the title ~~[WIP] added check for circular dependencies~~ Added check for circular dependencies Apr 20, 2018

jmarolf reviewed Apr 20, 2018

View reviewed changes

Pilchie reviewed Apr 20, 2018

View reviewed changes

Pilchie approved these changes Apr 21, 2018

View reviewed changes

jmarolf reviewed Apr 21, 2018

View reviewed changes

Pilchie added this to the 15.7 milestone Apr 23, 2018

Ethan Byrd added 4 commits April 24, 2018 10:26

changed scope of seenDependencies hashSet

2feb5ed

Better implementation with tests

1091f60

More descriptive variable names and comments for clarity

f7a3e3d

review fixes 1

aea2b93

etbyrd force-pushed the circular-dependency-check branch from fcf4fb4 to aea2b93 Compare April 24, 2018 17:33

etbyrd changed the base branch from master to dev15.7.x April 24, 2018 17:41

removed backups

52fa309

Pilchie merged commit dbe6534 into dotnet:dev15.7.x Apr 24, 2018

etbyrd mentioned this pull request Apr 25, 2018

Visual Studio hangs and then crashes when opening a solution with a circular package graph #3374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added check for circular dependencies #3474

Added check for circular dependencies #3474

etbyrd commented Apr 18, 2018

codecov-io commented Apr 18, 2018 •

edited

Loading

jmarolf commented Apr 18, 2018

abpiskunov Apr 18, 2018

etbyrd Apr 18, 2018

abpiskunov commented Apr 18, 2018

etbyrd commented Apr 18, 2018

Pilchie Apr 19, 2018

abpiskunov left a comment

jmarolf Apr 20, 2018

etbyrd Apr 21, 2018

Pilchie Apr 20, 2018

Pilchie Apr 20, 2018

Pilchie Apr 20, 2018

Pilchie Apr 20, 2018

etbyrd Apr 20, 2018

jmarolf Apr 21, 2018

etbyrd Apr 22, 2018

		@@ -55,6 +55,8 @@ internal class TargetedDependenciesSnapshot : ITargetedDependenciesSnapshot
		private Dictionary<string, bool> _unresolvedDescendantsMap
		= new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase);

Added check for circular dependencies #3474

Added check for circular dependencies #3474

Conversation

etbyrd commented Apr 18, 2018

codecov-io commented Apr 18, 2018 • edited Loading

Codecov Report

jmarolf commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abpiskunov commented Apr 18, 2018

etbyrd commented Apr 18, 2018

Choose a reason for hiding this comment

abpiskunov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 18, 2018 •

edited

Loading