From 1107227351bfe2052008413409067f03b28d9057 Mon Sep 17 00:00:00 2001 From: Sara Itani Date: Sun, 11 Oct 2015 16:48:38 -0700 Subject: [PATCH 1/2] #508 Unhandled System.ArgumentException and System.IndexOutOfRangeException during LoadCachedAnalysis - We were simultaneously mutating the AnalysisLog from two threads. This caused various Deque properties like _data, _tail, etc. to get out of sync with one another. The fix is to lock LogItems whenever adding new events to the log. --- .../Product/Analysis/Analysis/AnalysisLog.cs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/Nodejs/Product/Analysis/Analysis/AnalysisLog.cs b/Nodejs/Product/Analysis/Analysis/AnalysisLog.cs index bc7f0fd6a..e5aa30277 100644 --- a/Nodejs/Product/Analysis/Analysis/AnalysisLog.cs +++ b/Nodejs/Product/Analysis/Analysis/AnalysisLog.cs @@ -87,17 +87,19 @@ private void Add(string Event, params object[] Args) { int max = MaxItems; if (max != 0) { - if (LogItems.Count >= max) { - while (LogItems.Count > max) { - // if the queue was set to shrink remove any old items. - LogItems.PopLeft(); + lock (LogItems) { + if (LogItems.Count >= max) { + while (LogItems.Count > max) { + // if the queue was set to shrink remove any old items. + LogItems.PopLeft(); + } + if (LogIndex >= max) { + LogIndex = 0; + } + LogItems[LogIndex++] = new LogItem { Time = Time, Event = Event, Args = Args }; + } else { + LogItems.Append(new LogItem { Time = Time, Event = Event, Args = Args }); } - if (LogIndex >= max) { - LogIndex = 0; - } - LogItems[LogIndex++] = new LogItem { Time = Time, Event = Event, Args = Args }; - } else { - LogItems.Append(new LogItem { Time = Time, Event = Event, Args = Args }); } } } From d3c02979d9da024a2cc23befd741918644264675 Mon Sep 17 00:00:00 2001 From: Sara Itani Date: Tue, 13 Oct 2015 13:14:42 -0700 Subject: [PATCH 2/2] Move RequireAnalysisUnit instantiation to TreeUpdateAnalysis so that it is called on the analysis thread. --- .../Product/Analysis/Analysis/AnalysisLog.cs | 22 ++++++------- .../Analysis/Analyzer/AnalysisUnit.cs | 3 ++ .../Product/Analysis/Analysis/JsAnalyzer.cs | 32 ++++++++++++------- 3 files changed, 33 insertions(+), 24 deletions(-) diff --git a/Nodejs/Product/Analysis/Analysis/AnalysisLog.cs b/Nodejs/Product/Analysis/Analysis/AnalysisLog.cs index e5aa30277..bc7f0fd6a 100644 --- a/Nodejs/Product/Analysis/Analysis/AnalysisLog.cs +++ b/Nodejs/Product/Analysis/Analysis/AnalysisLog.cs @@ -87,19 +87,17 @@ private void Add(string Event, params object[] Args) { int max = MaxItems; if (max != 0) { - lock (LogItems) { - if (LogItems.Count >= max) { - while (LogItems.Count > max) { - // if the queue was set to shrink remove any old items. - LogItems.PopLeft(); - } - if (LogIndex >= max) { - LogIndex = 0; - } - LogItems[LogIndex++] = new LogItem { Time = Time, Event = Event, Args = Args }; - } else { - LogItems.Append(new LogItem { Time = Time, Event = Event, Args = Args }); + if (LogItems.Count >= max) { + while (LogItems.Count > max) { + // if the queue was set to shrink remove any old items. + LogItems.PopLeft(); } + if (LogIndex >= max) { + LogIndex = 0; + } + LogItems[LogIndex++] = new LogItem { Time = Time, Event = Event, Args = Args }; + } else { + LogItems.Append(new LogItem { Time = Time, Event = Event, Args = Args }); } } } diff --git a/Nodejs/Product/Analysis/Analysis/Analyzer/AnalysisUnit.cs b/Nodejs/Product/Analysis/Analysis/Analyzer/AnalysisUnit.cs index 7cf38ec99..d7ad94cfe 100644 --- a/Nodejs/Product/Analysis/Analysis/Analyzer/AnalysisUnit.cs +++ b/Nodejs/Product/Analysis/Analysis/Analyzer/AnalysisUnit.cs @@ -32,6 +32,9 @@ namespace Microsoft.NodejsTools.Analysis { /// Our dependency tracking scheme works by tracking analysis units - when we add a dependency it is the current /// AnalysisUnit which is dependent upon the variable. If the value of a variable changes then all of the dependent /// AnalysisUnit's will be re-enqueued. This proceeds until we reach a fixed point. + /// + /// Note that AnalysisUnit contructors / methods are not threadsafe because we expect there to be many AnalysisUnits, + /// and we want to keep things as performant as possible (which means minimizing locking behavior and whatnot.) /// [Serializable] internal class AnalysisUnit : ISet { diff --git a/Nodejs/Product/Analysis/Analysis/JsAnalyzer.cs b/Nodejs/Product/Analysis/Analysis/JsAnalyzer.cs index c45144e58..f8a2eecef 100644 --- a/Nodejs/Product/Analysis/Analysis/JsAnalyzer.cs +++ b/Nodejs/Product/Analysis/Analysis/JsAnalyzer.cs @@ -143,15 +143,12 @@ public IAnalyzable AddPackageJson(string filePath, string entryPoint, List(); + ProjectEntry projectEntry = null; if (dependencies != null) { - var projectEntry = new ProjectEntry(this, filePath, null); - requireAnalysisUnits.AddRange(dependencies.Select( - dependency => { return new RequireAnalysisUnit(tree, Modules, projectEntry, dependency); - })); + projectEntry = new ProjectEntry(this, filePath, null); } - return new TreeUpdateAnalysis(tree, requireAnalysisUnits); + return new TreeUpdateAnalysis(tree, dependencies, projectEntry, Modules); } /// @@ -162,22 +159,33 @@ public IAnalyzable AddPackageJson(string filePath, string entryPoint, List _requireAnalysisUnits; + private readonly IEnumerable _dependencies; + private readonly ProjectEntry _projectEntry; + private readonly ModuleTable _modules; - public TreeUpdateAnalysis(ModuleTree tree, IEnumerable requireAnalysisUnits = null) { + public TreeUpdateAnalysis(ModuleTree tree, IEnumerable dependencies = null, ProjectEntry projectEntry = null, ModuleTable modules = null) { _tree = tree; - _requireAnalysisUnits = requireAnalysisUnits; + _dependencies = dependencies; + _projectEntry = projectEntry; + _modules = modules; } public void Analyze(CancellationToken cancel) { if (_tree != null) { _tree.EnqueueDependents(); } - if (_requireAnalysisUnits != null) { - foreach (var unit in _requireAnalysisUnits) { + + if (_dependencies != null) { + var requireAnalysisUnits = new List(); + requireAnalysisUnits.AddRange(_dependencies.Select( + dependency => { + return new RequireAnalysisUnit(_tree, _modules, _projectEntry, dependency); + })); + + foreach (var unit in requireAnalysisUnits) { unit.AnalyzeWorker(null, cancel); } - } + } } }