From 47cb546645da0e283b578212c15ad0b595b58cf2 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 22 Apr 2016 12:07:13 -0700 Subject: [PATCH 1/6] Fix dispose pattern in a few more cases Just using the VS analyizer to find objects that are not being disposed properly and fixing some of these. Related to #870. Still trying to track down who is holding onto the NodeJsProject node after it is unloaded. --- .../Nodejs/Intellisense/AnalysisQueue.cs | 5 ++- .../Nodejs/Intellisense/BufferParser.cs | 38 +++++++++---------- .../Nodejs/Intellisense/VsProjectAnalyzer.cs | 7 ++++ .../Product/Nodejs/Project/NodeModulesNode.cs | 4 ++ 4 files changed, 34 insertions(+), 20 deletions(-) diff --git a/Nodejs/Product/Nodejs/Intellisense/AnalysisQueue.cs b/Nodejs/Product/Nodejs/Intellisense/AnalysisQueue.cs index 924021e19..94afa461b 100644 --- a/Nodejs/Product/Nodejs/Intellisense/AnalysisQueue.cs +++ b/Nodejs/Product/Nodejs/Intellisense/AnalysisQueue.cs @@ -39,7 +39,7 @@ sealed class AnalysisQueue : IDisposable { private readonly HashSet _enqueuedGroups; [NonSerialized] private DateTime _lastSave; - private CancellationTokenSource _cancel; + private readonly CancellationTokenSource _cancel; private bool _isAnalyzing; private int _analysisPending; private static readonly TimeSpan _SaveAnalysisTime = TimeSpan.FromMinutes(15); @@ -137,6 +137,9 @@ public int AnalysisPending { void IDisposable.Dispose() { Stop(); + + _cancel.Dispose(); + _workEvent.Dispose(); } #endregion diff --git a/Nodejs/Product/Nodejs/Intellisense/BufferParser.cs b/Nodejs/Product/Nodejs/Intellisense/BufferParser.cs index 13777e73a..033871bcc 100644 --- a/Nodejs/Product/Nodejs/Intellisense/BufferParser.cs +++ b/Nodejs/Product/Nodejs/Intellisense/BufferParser.cs @@ -16,21 +16,15 @@ using System; using System.Collections.Generic; -using System.Diagnostics; -using System.IO; -using System.IO.Compression; using System.Linq; using System.Threading; -using System.Windows; -using System.Windows.Threading; using Microsoft.NodejsTools.Analysis; using Microsoft.VisualStudio.Text; -using Microsoft.VisualStudio.Text.Editor; namespace Microsoft.NodejsTools.Intellisense { sealed partial class VsProjectAnalyzer { - class BufferParser { + class BufferParser : IDisposable { internal VsProjectAnalyzer _parser; private readonly Timer _timer; private IList _buffers; @@ -51,18 +45,6 @@ public BufferParser(IProjectEntry initialProjectEntry, VsProjectAnalyzer parser, InitBuffer(buffer); } - public void StopMonitoring() { - foreach (var buffer in _buffers) { - buffer.ChangedLowPriority -= BufferChangedLowPriority; - buffer.Properties.RemoveProperty(typeof(BufferParser)); - if (_document != null) { - _document.EncodingChanged -= EncodingChanged; - _document = null; - } - } - _timer.Dispose(); - } - public ITextBuffer[] Buffers { get { return _buffers.ToArray(); @@ -270,6 +252,24 @@ internal ITextDocument Document { return _document; } } + + #region IDisposable + private bool disposedValue = false; + + protected virtual void Dispose(bool disposing) { + if (!disposedValue) { + if (disposing) { + _timer.Dispose(); + } + + disposedValue = true; + } + } + + public void Dispose() { + Dispose(true); + } + #endregion } } } diff --git a/Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs b/Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs index b913c04b5..ff0a2797f 100644 --- a/Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs +++ b/Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs @@ -1330,6 +1330,13 @@ public void Dispose() { _analysisQueue.Stop(); } + foreach (var bufferParser in _activeBufferParsers) { + bufferParser.Dispose(); + } + _activeBufferParsers.Clear(); + + _queueActivityEvent.Dispose(); + TaskProvider.Dispose(); } diff --git a/Nodejs/Product/Nodejs/Project/NodeModulesNode.cs b/Nodejs/Product/Nodejs/Project/NodeModulesNode.cs index da3e6298f..d30721835 100644 --- a/Nodejs/Product/Nodejs/Project/NodeModulesNode.cs +++ b/Nodejs/Product/Nodejs/Project/NodeModulesNode.cs @@ -103,6 +103,10 @@ protected override void Dispose(bool disposing) { _npmController.CommandCompleted -= NpmController_CommandCompleted; } + _devModulesNode.Dispose(); + _optionalModulesNode.Dispose(); + _globalModulesNode.Dispose(); + _isDisposed = true; } From eaa7d2923b0877e99f76f31d5970def7bd20bc44 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 22 Apr 2016 12:22:57 -0700 Subject: [PATCH 2/6] Fix a few more cases --- .../Classifier/SnapshotSpanSourceCodeReader.cs | 2 ++ .../Communication/WebSocketNetworkClient.cs | 1 + .../Product/Nodejs/Intellisense/AnalysisQueue.cs | 2 +- .../Nodejs/Intellisense/VsProjectAnalyzer.cs | 2 +- Nodejs/Product/Nodejs/NodejsEditorFactory.cs | 16 +--------------- 5 files changed, 6 insertions(+), 17 deletions(-) diff --git a/Nodejs/Product/Nodejs/Classifier/SnapshotSpanSourceCodeReader.cs b/Nodejs/Product/Nodejs/Classifier/SnapshotSpanSourceCodeReader.cs index 29dbe63e7..9d71b5ea5 100644 --- a/Nodejs/Product/Nodejs/Classifier/SnapshotSpanSourceCodeReader.cs +++ b/Nodejs/Product/Nodejs/Classifier/SnapshotSpanSourceCodeReader.cs @@ -42,6 +42,8 @@ public override void Close() { protected override void Dispose(bool disposing) { _snapshot = null; _position = 0; + + base.Dispose(disposing); } public override int Peek() { diff --git a/Nodejs/Product/Nodejs/Debugger/Communication/WebSocketNetworkClient.cs b/Nodejs/Product/Nodejs/Debugger/Communication/WebSocketNetworkClient.cs index 73149c13a..6f5ffa2bb 100644 --- a/Nodejs/Product/Nodejs/Debugger/Communication/WebSocketNetworkClient.cs +++ b/Nodejs/Product/Nodejs/Debugger/Communication/WebSocketNetworkClient.cs @@ -49,6 +49,7 @@ public bool Connected { } public void Dispose() { + _stream.Dispose(); try { _webSocket.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, null, CancellationToken.None).GetAwaiter().GetResult(); _webSocket.Dispose(); diff --git a/Nodejs/Product/Nodejs/Intellisense/AnalysisQueue.cs b/Nodejs/Product/Nodejs/Intellisense/AnalysisQueue.cs index 94afa461b..b6e2d43e9 100644 --- a/Nodejs/Product/Nodejs/Intellisense/AnalysisQueue.cs +++ b/Nodejs/Product/Nodejs/Intellisense/AnalysisQueue.cs @@ -135,7 +135,7 @@ public int AnalysisPending { #region IDisposable Members - void IDisposable.Dispose() { + public void Dispose() { Stop(); _cancel.Dispose(); diff --git a/Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs b/Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs index ff0a2797f..9955f5231 100644 --- a/Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs +++ b/Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs @@ -1327,7 +1327,7 @@ public void Dispose() { } if (_analysisQueue != null) { - _analysisQueue.Stop(); + _analysisQueue.Dispose(); } foreach (var bufferParser in _activeBufferParsers) { diff --git a/Nodejs/Product/Nodejs/NodejsEditorFactory.cs b/Nodejs/Product/Nodejs/NodejsEditorFactory.cs index 1669a2473..cd9a8b835 100644 --- a/Nodejs/Product/Nodejs/NodejsEditorFactory.cs +++ b/Nodejs/Product/Nodejs/NodejsEditorFactory.cs @@ -123,24 +123,10 @@ public virtual int MapLogicalView(ref Guid logicalView, out string physicalView) } public virtual int Close() { + _serviceProvider?.Dispose(); return VSConstants.S_OK; } - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// public virtual int CreateEditorInstance( uint createEditorFlags, string documentMoniker, From 2c7e746643c4800a4d5c5eea9f66ff62ebc5fab2 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 22 Apr 2016 12:39:42 -0700 Subject: [PATCH 3/6] Use weak references to reduce some cycles --- Common/Product/SharedProject/ProjectNode.cs | 39 +++++++++++++-------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/Common/Product/SharedProject/ProjectNode.cs b/Common/Product/SharedProject/ProjectNode.cs index 27c7a09de..a29774975 100644 --- a/Common/Product/SharedProject/ProjectNode.cs +++ b/Common/Product/SharedProject/ProjectNode.cs @@ -278,7 +278,7 @@ internal enum EventTriggering { /// /// Mapping from item names to their hierarchy nodes for all disk-based nodes. /// - protected readonly Dictionary _diskNodes = new Dictionary(StringComparer.OrdinalIgnoreCase); + protected readonly Dictionary> _diskNodes = new Dictionary>(StringComparer.OrdinalIgnoreCase); // Has the object been disposed. private bool isDisposed; @@ -292,7 +292,7 @@ internal enum EventTriggering { /// is added, then the label is edited, and when that completes/cancels /// the folder gets created. /// - private FolderNode _folderBeingCreated; + private WeakReference _folderBeingCreated; private readonly ExtensibilityEventsDispatcher extensibilityEventsDispatcher; @@ -404,10 +404,14 @@ internal ExtensibilityEventsDispatcher ExtensibilityEventsDispatcher { /// internal FolderNode FolderBeingCreated { get { - return _folderBeingCreated; + FolderNode node = null; + if (_folderBeingCreated.TryGetTarget(out node)) { + return node; + } + return null; } set { - _folderBeingCreated = value; + _folderBeingCreated = value == null ? null : new WeakReference(value); } } @@ -1579,7 +1583,7 @@ public virtual void Load(string fileName, string location, string name, uint fla } else { this.filename = fileName; } - _diskNodes[this.filename] = this; + _diskNodes[this.filename] = new WeakReference(this); // now reload to fix up references this.Reload(); @@ -2596,7 +2600,7 @@ protected virtual void SaveMSBuildProjectFileAs(string newFileName) { _diskNodes.Remove(this.filename); this.filename = newFileName; - _diskNodes[this.filename] = this; + _diskNodes[this.filename] = new WeakReference(this); string newFileNameWithoutExtension = Path.GetFileNameWithoutExtension(newFileName); @@ -5040,10 +5044,13 @@ internal HierarchyNode GetItemParentNode(MSBuild.ProjectItem item) { if (!String.IsNullOrWhiteSpace(link)) { strPath = Path.GetDirectoryName(link); } else { - HierarchyNode parent; - if (_diskNodes.TryGetValue(Path.GetDirectoryName(Path.Combine(ProjectHome, strPath)) + "\\", out parent)) { - // fast path, filename is normalized, and the folder already exists - return parent; + WeakReference parentRef; + if (_diskNodes.TryGetValue(Path.GetDirectoryName(Path.Combine(ProjectHome, strPath)) + "\\", out parentRef)) { + HierarchyNode parent; + if (parentRef.TryGetTarget(out parent)) { + // fast path, filename is normalized, and the folder already exists + return parent; + } } string absPath = CommonUtils.GetAbsoluteFilePath(ProjectHome, strPath); @@ -5455,9 +5462,13 @@ internal HierarchyNode FindNodeByFullPath(string name) { Debug.Assert(Path.IsPathRooted(name)); - HierarchyNode res; - _diskNodes.TryGetValue(name, out res); - return res; + WeakReference res; + if (_diskNodes.TryGetValue(name, out res)) { + HierarchyNode node = null; + res.TryGetTarget(out node); + return node; + } + return null; } /// @@ -5665,7 +5676,7 @@ internal void OnItemAdded(HierarchyNode parent, HierarchyNode child, HierarchyNo IDiskBasedNode diskNode = child as IDiskBasedNode; if (diskNode != null) { - _diskNodes[diskNode.Url] = child; + _diskNodes[diskNode.Url] = new WeakReference(child); } if ((EventTriggeringFlag & ProjectNode.EventTriggering.DoNotTriggerHierarchyEvents) != 0) { From fab587248b3f92b894e4137c8f005fd20e15a3c7 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 22 Apr 2016 13:39:16 -0700 Subject: [PATCH 4/6] Clear analysis queue on dispose --- Nodejs/Product/Nodejs/Project/NodejsProjectNode.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Nodejs/Product/Nodejs/Project/NodejsProjectNode.cs b/Nodejs/Product/Nodejs/Project/NodejsProjectNode.cs index efeccb90b..374da3bcc 100644 --- a/Nodejs/Product/Nodejs/Project/NodejsProjectNode.cs +++ b/Nodejs/Product/Nodejs/Project/NodejsProjectNode.cs @@ -55,8 +55,8 @@ class NodejsProjectNode : CommonProjectNode, VsWebSite.VSWebSite, INodePackageMo #endif // We delay analysis until things calm down in the node_modules folder. - internal Queue DelayedAnalysisQueue = new Queue(); - private object _idleNodeModulesLock = new object(); + internal readonly Queue DelayedAnalysisQueue = new Queue(); + private readonly object _idleNodeModulesLock = new object(); private volatile bool _isIdleNodeModules = false; private Timer _idleNodeModulesTimer; @@ -1016,6 +1016,8 @@ protected override void Dispose(bool disposing) { RemoveChild(ModulesNode); ModulesNode?.Dispose(); ModulesNode = null; + + DelayedAnalysisQueue.Clear(); #if DEV14 _typingsAcquirer = null; #endif From 68603483596ea4d8874a4866908c48518233049d Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 22 Apr 2016 13:46:42 -0700 Subject: [PATCH 5/6] Manually cut NodejsProjectNode -> NodeJsFolderNode reference link --- Common/Product/SharedProject/HierarchyNode.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Common/Product/SharedProject/HierarchyNode.cs b/Common/Product/SharedProject/HierarchyNode.cs index aa8ffc246..25e6eb1ad 100644 --- a/Common/Product/SharedProject/HierarchyNode.cs +++ b/Common/Product/SharedProject/HierarchyNode.cs @@ -1564,6 +1564,10 @@ protected virtual void OnCancelLabelEdit() { /// /// Is the Dispose called by some internal member, or it is called by from GC. protected virtual void Dispose(bool disposing) { + firstChild = null; + lastChild = null; + nextSibling = null; + itemNode = null; } /// From 271ed00d6110d2a722a63a9a516670692640deaa Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 6 May 2016 14:54:16 -0700 Subject: [PATCH 6/6] Remove weakref stuff in favor of nulling out values on dispose --- Common/Product/SharedProject/ProjectNode.cs | 42 +++++++++------------ 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/Common/Product/SharedProject/ProjectNode.cs b/Common/Product/SharedProject/ProjectNode.cs index 076020168..1b0e799c9 100644 --- a/Common/Product/SharedProject/ProjectNode.cs +++ b/Common/Product/SharedProject/ProjectNode.cs @@ -278,7 +278,7 @@ internal enum EventTriggering { /// /// Mapping from item names to their hierarchy nodes for all disk-based nodes. /// - protected readonly Dictionary> _diskNodes = new Dictionary>(StringComparer.OrdinalIgnoreCase); + protected readonly Dictionary _diskNodes = new Dictionary(StringComparer.OrdinalIgnoreCase); // Has the object been disposed. private bool isDisposed; @@ -292,7 +292,7 @@ internal enum EventTriggering { /// is added, then the label is edited, and when that completes/cancels /// the folder gets created. /// - private WeakReference _folderBeingCreated; + private FolderNode _folderBeingCreated; private readonly ExtensibilityEventsDispatcher extensibilityEventsDispatcher; @@ -404,14 +404,10 @@ internal ExtensibilityEventsDispatcher ExtensibilityEventsDispatcher { /// internal FolderNode FolderBeingCreated { get { - FolderNode node = null; - if (_folderBeingCreated.TryGetTarget(out node)) { - return node; - } - return null; + return _folderBeingCreated; } set { - _folderBeingCreated = value == null ? null : new WeakReference(value); + _folderBeingCreated = value; } } @@ -1128,6 +1124,9 @@ protected override void Dispose(bool disposing) { imageHandler.Close(); imageHandler = null; } + + _diskNodes.Clear(); + _folderBeingCreated = null; } finally { base.Dispose(disposing); // Note that this isDisposed flag is separate from the base's @@ -1583,7 +1582,7 @@ public virtual void Load(string fileName, string location, string name, uint fla } else { this.filename = fileName; } - _diskNodes[this.filename] = new WeakReference(this); + _diskNodes[this.filename] = this; // now reload to fix up references this.Reload(); @@ -2606,7 +2605,7 @@ protected virtual void SaveMSBuildProjectFileAs(string newFileName) { _diskNodes.Remove(this.filename); this.filename = newFileName; - _diskNodes[this.filename] = new WeakReference(this); + _diskNodes[this.filename] = this; string newFileNameWithoutExtension = Path.GetFileNameWithoutExtension(newFileName); @@ -5060,13 +5059,10 @@ internal HierarchyNode GetItemParentNode(MSBuild.ProjectItem item) { if (!String.IsNullOrWhiteSpace(link)) { strPath = Path.GetDirectoryName(link); } else { - WeakReference parentRef; - if (_diskNodes.TryGetValue(Path.GetDirectoryName(Path.Combine(ProjectHome, strPath)) + "\\", out parentRef)) { - HierarchyNode parent; - if (parentRef.TryGetTarget(out parent)) { - // fast path, filename is normalized, and the folder already exists - return parent; - } + HierarchyNode parent; + if (_diskNodes.TryGetValue(Path.GetDirectoryName(Path.Combine(ProjectHome, strPath)) + "\\", out parent)) { + // fast path, filename is normalized, and the folder already exists + return parent; } string absPath = CommonUtils.GetAbsoluteFilePath(ProjectHome, strPath); @@ -5478,13 +5474,9 @@ internal HierarchyNode FindNodeByFullPath(string name) { Debug.Assert(Path.IsPathRooted(name)); - WeakReference res; - if (_diskNodes.TryGetValue(name, out res)) { - HierarchyNode node = null; - res.TryGetTarget(out node); - return node; - } - return null; + HierarchyNode node; + _diskNodes.TryGetValue(name, out node); + return node; } /// @@ -5692,7 +5684,7 @@ internal void OnItemAdded(HierarchyNode parent, HierarchyNode child, HierarchyNo IDiskBasedNode diskNode = child as IDiskBasedNode; if (diskNode != null) { - _diskNodes[diskNode.Url] = new WeakReference(child); + _diskNodes[diskNode.Url] = child; } if ((EventTriggeringFlag & ProjectNode.EventTriggering.DoNotTriggerHierarchyEvents) != 0) {