Merge pull request #1933 from paulvanbrenk/removeNodeCrash

Remove node crash

Author: paulvanbrenk

Common/Product/SharedProject/CommonUtils.cs

@@ -550,7 +550,8 @@ public static string EnsureEndSeparator(string path)
         {
             if (string.IsNullOrEmpty(path))
             {
-                return string.Empty;
+                Debug.Fail("what??");
+                return path;
             }
             else if (!HasEndSeparator(path))
             {

Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.DiskMerger.cs

@@ -59,7 +59,7 @@ public async Task<bool> ContinueMergeAsync(bool hierarchyCreated)
             private async Task<bool> ContinueMergeAsyncWorker((string Name, HierarchyNode Parent) dir, bool hierarchyCreated)
             {
                 var wasExpanded = hierarchyCreated ? dir.Parent.GetIsExpanded() : false;
-                var missingChildren = new HashSet<HierarchyNode>(dir.Parent.AllChildren);
+                var missingOnDisk = new HashSet<HierarchyNode>(dir.Parent.AllChildren);
 
                 var thread = this.project.Site.GetUIThread();
 
@@ -92,15 +92,9 @@ private async Task<bool> ContinueMergeAsyncWorker((string Name, HierarchyNode Pa
                             this.project.CreateSymLinkWatcher(curDir);
                         }
 
-                        var existing = this.project.FindNodeByFullPath(curDir);
-                        if (existing == null)
-                        {
-                            existing = this.project.AddAllFilesFolder(dir.Parent, curDir + Path.DirectorySeparatorChar, hierarchyCreated);
-                        }
-                        else
-                        {
-                            missingChildren.Remove(existing);
-                        }
+                        var existing = this.project.AddAllFilesFolder(dir.Parent, curDir, hierarchyCreated);
+                        missingOnDisk.Remove(existing);
+
                         this.remainingDirs.Push((curDir, existing));
                     }
                 }
@@ -136,7 +130,7 @@ private async Task<bool> ContinueMergeAsyncWorker((string Name, HierarchyNode Pa
                         }
                         else
                         {
-                            missingChildren.Remove(existing);
+                            missingOnDisk.Remove(existing);
                         }
                     }
 
@@ -156,7 +150,7 @@ private async Task<bool> ContinueMergeAsyncWorker((string Name, HierarchyNode Pa
                 }
 
                 // remove the excluded children which are no longer there
-                this.RemoveMissingChildren(missingChildren);
+                this.RemoveMissingChildren(missingOnDisk);
 
                 if (hierarchyCreated)
                 {

Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.FileSystemChange.cs

@@ -184,7 +184,7 @@ private async Task ChildCreatedAsync(HierarchyNode child)
                         this.project.CreateSymLinkWatcher(this.path);
                     }
 
-                    var folderNode = this.project.AddAllFilesFolder(parent, this.path + Path.DirectorySeparatorChar);
+                    var folderNode = this.project.AddAllFilesFolder(parent, this.path);
                     var folderNodeWasExpanded = folderNode.GetIsExpanded();
 
                     // then add the folder nodes

Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.cs

@@ -693,10 +693,12 @@ private HierarchyNode AddAllFilesFile(HierarchyNode curParent, string file)
         /// </summary>
         private HierarchyNode AddAllFilesFolder(HierarchyNode curParent, string curDir, bool hierarchyCreated = true)
         {
-            var folderNode = FindNodeByFullPath(curDir);
+            var safePath = CommonUtils.EnsureEndSeparator(curDir);
+
+            var folderNode = FindNodeByFullPath(safePath);
             if (folderNode == null)
             {
-                folderNode = CreateFolderNode(new AllFilesProjectElement(curDir, "Folder", this));
+                folderNode = CreateFolderNode(new AllFilesProjectElement(safePath, "Folder", this));
                 AddAllFilesNode(curParent, folderNode);
 
                 if (hierarchyCreated)

Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs

@@ -1,25 +1,29 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
 using System;
-using System.Collections.Generic;
+using System.Collections.Concurrent;
 using System.Diagnostics;
 
 namespace Microsoft.VisualStudioTools.Project
 {
     internal sealed class HierarchyIdMap
     {
-        private readonly List<WeakReference<HierarchyNode>> ids = new List<WeakReference<HierarchyNode>>();
-        private readonly Stack<int> freedIds = new Stack<int>();
+        private readonly ConcurrentDictionary<uint, WeakReference<HierarchyNode>> nodes = new ConcurrentDictionary<uint, WeakReference<HierarchyNode>>();
+        private readonly ConcurrentStack<uint> freedIds = new ConcurrentStack<uint>();
 
-        private readonly object theLock = new object();
+        public readonly static HierarchyIdMap Instance = new HierarchyIdMap();
+
+        private HierarchyIdMap() { }
 
         /// <summary>
         /// Must be called from the UI thread
         /// </summary>
         public uint Add(HierarchyNode node)
         {
+            VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread();
+            Debug.Assert(node != null, "The node added here should never be null.");
 #if DEBUG
-            foreach (var reference in this.ids)
+            foreach (var reference in this.nodes.Values)
             {
                 if (reference != null)
                 {
@@ -30,24 +34,21 @@ public uint Add(HierarchyNode node)
                 }
             }
 #endif
-
-            VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread();
-
-            lock (this.theLock)
+            if (!this.freedIds.TryPop(out var idx))
             {
-                if (this.freedIds.Count > 0)
-                {
-                    var i = this.freedIds.Pop();
-                    this.ids[i] = new WeakReference<HierarchyNode>(node);
-                    return (uint)i + 1;
-                }
-                else
-                {
-                    this.ids.Add(new WeakReference<HierarchyNode>(node));
-                    // ids are 1 based
-                    return (uint)this.ids.Count;
-                }
+                idx = this.NextIndex();
             }
+
+            var addSuccess = this.nodes.TryAdd(idx, new WeakReference<HierarchyNode>(node));
+            Debug.Assert(addSuccess, "Failed to add a new item");
+
+            return idx;
+        }
+
+        private uint NextIndex()
+        {
+            // +1 since 0 is not a valid HierarchyId
+            return (uint)this.nodes.Count + 1;
         }
 
         /// <summary>
@@ -57,33 +58,17 @@ public void Remove(HierarchyNode node)
         {
             VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread();
 
-            if (node == null)
-            {
-                throw new ArgumentNullException(nameof(node));
-            }
+            Debug.Assert(node != null, "Called with null node");
 
-            lock (this.theLock)
-            {
-                var i = (int)node.ID - 1;
-                if (0 > i || i >= this.ids.Count)
-                {
-                    throw new InvalidOperationException($"Invalid id. {i}");
-                }
+            var idx = node.ID;
 
-                var weakRef = this.ids[i];
-                if (weakRef == null)
-                {
-                    throw new InvalidOperationException("Trying to retrieve a node before adding.");
-                }
+            var removeCheck = this.nodes.TryRemove(idx, out var weakRef);
 
-                if (weakRef.TryGetTarget(out var found) && !object.ReferenceEquals(node, found))
-                {
-                    throw new InvalidOperationException("The node has the wrong id.");
-                }
+            Debug.Assert(removeCheck, "How did we get an id, which we haven't seen before");
+            Debug.Assert(weakRef != null, "How did we insert a null value.");
+            Debug.Assert(weakRef.TryGetTarget(out var found) && object.ReferenceEquals(node, found), "The node has the wrong id, or was GC-ed before.");
 
-                this.ids[i] = null;
-                this.freedIds.Push(i);
-            }
+            this.freedIds.Push(idx);
         }
 
         /// <summary>
@@ -93,15 +78,16 @@ public HierarchyNode this[uint itemId]
         {
             get
             {
-                var i = (int)itemId - 1;
-                if (0 <= i && i < this.ids.Count)
+                VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread();
+
+                var idx = itemId;
+                if (this.nodes.TryGetValue(idx, out var reference) && reference != null && reference.TryGetTarget(out var node))
                 {
-                    var reference = this.ids[i];
-                    if (reference != null && reference.TryGetTarget(out var node))
-                    {
-                        return node;
-                    }
+                    Debug.Assert(node != null);
+                    return node;
                 }
+
+                // This is a valid return value, this gets called by VS after we deleted the item
                 return null;
             }
         }

Nodejs/Product/Nodejs/SharedProject/ProjectNode.cs

@@ -158,9 +158,6 @@ internal enum EventTriggering
         /// <summary>A project will only try to build if it can obtain a lock on this object</summary>
         private volatile static object BuildLock = new object();
 
-        /// <summary>Maps integer ids to project item instances</summary>
-        private HierarchyIdMap itemIdMap = new HierarchyIdMap();
-
         /// <summary>A service provider call back object provided by the IDE hosting the project manager</summary>
         private IServiceProvider site;
 
@@ -590,7 +587,7 @@ public virtual string OutputBaseRelativePath
         /// <summary>
         /// Gets a collection of integer ids that maps to project item instances
         /// </summary>
-        internal HierarchyIdMap ItemIdMap => this.itemIdMap;
+        internal HierarchyIdMap ItemIdMap => HierarchyIdMap.Instance;
 
         /// <summary>
         /// Get the helper object that track document changes.