Skip to content
This repository has been archived by the owner on Apr 14, 2022. It is now read-only.

Fix modules reload leak #182

Closed
wants to merge 16 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,15 @@ public static StringBuilder EnsureEndsWithWhiteSpace(this StringBuilder sb, int
if (whiteSpaceCount > 0) {
sb.Append(' ', whiteSpaceCount);
}


return sb;
}

public static StringBuilder AppendIf(this StringBuilder sb, bool condition, string value) {
if (condition) {
sb.Append(value);
}

return sb;
}
}
Expand Down
17 changes: 11 additions & 6 deletions src/Analysis/Engine/Impl/Interpreter/Ast/AstPythonInterpreter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ public void Initialize(PythonAnalyzer state) {
}

_analyzer = state;
RemoveImportedModules();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a lock here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code locks on _userSearchPathImported operations

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am resolving it on another work with importing/search paths


if (state != null) {
lock (_userSearchPathsLock) {
Expand All @@ -289,19 +290,23 @@ public void Initialize(PythonAnalyzer state) {

private void Analyzer_SearchPathsChanged(object sender, EventArgs e) {
lock (_userSearchPathsLock) {
// Remove imported modules from search paths so we will
// import them again.
foreach (var name in _userSearchPathImported.MaybeEnumerate()) {
IPythonModule mod;
_modules.TryRemove(name, out mod);
}
RemoveImportedModules();
_userSearchPathImported = null;
_userSearchPathPackages = null;
_userSearchPaths = _analyzer.GetSearchPaths();
}
ModuleNamesChanged?.Invoke(this, EventArgs.Empty);
}

private void RemoveImportedModules() {
lock (_userSearchPathsLock) {
// Remove imported modules from search paths so we will import them again.
foreach (var name in _userSearchPathImported.MaybeEnumerate()) {
_modules.TryRemove(name, out var mod);
}
}
}

public IEnumerable<string> GetModulesNamed(string name) {
var usp = GetUserSearchPathPackagesAsync(CancellationToken.None).WaitAndUnwrapExceptions();
var ssp = _factory.GetImportableModulesAsync(CancellationToken.None).WaitAndUnwrapExceptions();
Expand Down
2 changes: 1 addition & 1 deletion src/Analysis/Engine/Impl/ModuleTable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public ModuleReference GetOrAdd(string name) {
/// Modules that are already in the table as builtins are replaced or
/// removed, but no new modules are added.
/// </summary>
public void ReInit() {
internal void Reload() {
var newNames = new HashSet<string>(_interpreter.GetModuleNames(), StringComparer.Ordinal);

foreach (var keyValue in _modules) {
Expand Down
6 changes: 3 additions & 3 deletions src/Analysis/Engine/Impl/PythonAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -174,16 +174,16 @@ private async Task LoadKnownTypesAsync(CancellationToken token) {
}

try {
_interpreterFactory.NotifyImportNamesChanged();
_modules.ReInit();
_modules.Reload();
_interpreter.Initialize(this);

await LoadKnownTypesAsync(token);

foreach (var mod in _modulesByFilename.Values) {
mod.Clear();
mod.EnsureModuleVariables(this);
}

_interpreterFactory.NotifyImportNamesChanged();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is listening to this ImportableModulesChanged event?

Copy link
Author

@MikhailArkhipov MikhailArkhipov Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AstFactory and AstIntepreter, erasing caches

} finally {
_reloadLock.Release();
}
Expand Down
39 changes: 32 additions & 7 deletions src/Analysis/Engine/Impl/Values/Protocols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.PythonTools.Analysis.Infrastructure;
using Microsoft.PythonTools.Interpreter;
using Microsoft.PythonTools.Parsing;
Expand Down Expand Up @@ -389,9 +390,32 @@ class TupleProtocol : IterableProtocol {

public TupleProtocol(ProtocolInfo self, IEnumerable<IAnalysisSet> values) : base(self, AnalysisSet.UnionAll(values)) {
_values = values.Select(s => s.AsUnion(1)).ToArray();
Name = GetNameFromValues();
}

private string GetNameFromValues() {
// Enumerate manually since SelectMany drops empty/unknown values
var sb = new StringBuilder("tuple[");
for (var i = 0; i < _values.Length; i++) {
sb.AppendIf(i > 0, ", ");
AppendParameterString(sb, _values[i].ToArray());
}
sb.Append(']');
return sb.ToString();
}

private void AppendParameterString(StringBuilder sb, AnalysisValue[] sets) {
if (sets.Length == 0) {
sb.Append('?');
return;
}

var argTypes = _values.SelectMany(e => e.Select(v => v is IHasQualifiedName qn ? qn.FullyQualifiedName : v.ShortDescription));
Name = "tuple[{0}]".FormatInvariant(string.Join(", ", argTypes));
sb.AppendIf(sets.Length > 1, "[");
for (var i = 0; i < sets.Length; i++) {
sb.AppendIf(i > 0, ", ");
sb.Append(sets[i] is IHasQualifiedName qn ? qn.FullyQualifiedName : sets[i].ShortDescription);
}
sb.AppendIf(sets.Length > 1, "]");
}

protected override void EnsureMembers(IDictionary<string, IAnalysisSet> members) {
Expand Down Expand Up @@ -419,6 +443,7 @@ public override IAnalysisSet GetIndex(Node node, AnalysisUnit unit, IAnalysisSet
}

public override string Name { get; }
public override BuiltinTypeId TypeId => BuiltinTypeId.Tuple;

public override IEnumerable<KeyValuePair<string, string>> GetRichDescription() {
if (_values.Any()) {
Expand All @@ -437,7 +462,7 @@ public override IEnumerable<KeyValuePair<string, string>> GetRichDescription() {
}
}

protected override bool Equals(Protocol other) =>
protected override bool Equals(Protocol other) =>
other is TupleProtocol tp &&
_values.Zip(tp._values, (x, y) => x.SetEquals(y)).All(b => b);

Expand Down Expand Up @@ -524,6 +549,7 @@ public override IAnalysisSet GetIndex(Node node, AnalysisUnit unit, IAnalysisSet
}

public override string Name => "dict";
public override BuiltinTypeId TypeId => BuiltinTypeId.Dict;

public override IEnumerable<KeyValuePair<string, string>> GetRichDescription() {
if (_valueType.Any()) {
Expand Down Expand Up @@ -576,14 +602,13 @@ protected override void EnsureMembers(IDictionary<string, IAnalysisSet> members)
}

public override string Name => "generator";

public override BuiltinTypeId TypeId => BuiltinTypeId.Generator;

public IAnalysisSet Yielded => _yielded;
public IAnalysisSet Sent { get; }
public IAnalysisSet Returned { get; }

public override IAnalysisSet GetReturnForYieldFrom(Node node, AnalysisUnit unit) {
return Returned;
}
public override IAnalysisSet GetReturnForYieldFrom(Node node, AnalysisUnit unit) => Returned;

public override IEnumerable<KeyValuePair<string, string>> GetRichDescription() {
if (_yielded.Any() || Sent.Any() || Returned.Any()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
using Microsoft.Python.LanguageServer.Extensions;
using Microsoft.PythonTools;
using Microsoft.PythonTools.Analysis;
using Microsoft.PythonTools.Analysis.Infrastructure;
using Microsoft.PythonTools.Parsing;
using Microsoft.PythonTools.Parsing.Ast;

Expand Down
22 changes: 8 additions & 14 deletions src/LanguageServer/Impl/Implementation/Server.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@ public sealed partial class Server : ServerBase, ILogger, IPythonLanguageServer,
/// Implements ability to execute module reload on the analyzer thread
/// </summary>
private sealed class ReloadModulesQueueItem : IAnalyzable {
private readonly PythonAnalyzer _analyzer;
private readonly Server _server;
private TaskCompletionSource<bool> _tcs = new TaskCompletionSource<bool>();
public Task Task => _tcs.Task;

public ReloadModulesQueueItem(PythonAnalyzer analyzer) {
_analyzer = analyzer;
public ReloadModulesQueueItem(Server server) {
_server = server;
}
public void Analyze(CancellationToken cancel) {
if (cancel.IsCancellationRequested) {
Expand All @@ -54,7 +54,10 @@ public void Analyze(CancellationToken cancel) {

var currentTcs = Interlocked.Exchange(ref _tcs, new TaskCompletionSource<bool>());
try {
_analyzer.ReloadModulesAsync(cancel).WaitAndUnwrapExceptions();
_server.Analyzer.ReloadModulesAsync(cancel).WaitAndUnwrapExceptions();
foreach (var entry in _server.Analyzer.ModulesByFilename) {
_server.AnalysisQueue.Enqueue(entry.Value.ProjectEntry, AnalysisPriority.Normal);
}
currentTcs.TrySetResult(true);
} catch (OperationCanceledException oce) {
currentTcs.TrySetCanceled(oce.CancellationToken);
Expand Down Expand Up @@ -374,7 +377,7 @@ private async Task DoInitializeAsync(InitializeParams @params, CancellationToken
_analysisUpdates = @params.initializationOptions.analysisUpdates;

Analyzer.EnableDiagnostics = _clientCaps?.python?.liveLinting ?? false;
_reloadModulesQueueItem = new ReloadModulesQueueItem(Analyzer);
_reloadModulesQueueItem = new ReloadModulesQueueItem(this);

if (@params.initializationOptions.displayOptions != null) {
DisplayOptions = @params.initializationOptions.displayOptions;
Expand All @@ -391,8 +394,6 @@ private async Task DoInitializeAsync(InitializeParams @params, CancellationToken

SetSearchPaths(@params.initializationOptions.searchPaths);
SetTypeStubSearchPaths(@params.initializationOptions.typeStubSearchPaths);

Analyzer.Interpreter.ModuleNamesChanged += Interpreter_ModuleNamesChanged;
}

private void DisplayStartupInfo() {
Expand All @@ -403,13 +404,6 @@ private void DisplayStartupInfo() {
: Resources.InitializingForPythonInterpreter.FormatInvariant(Analyzer.InterpreterFactory.Configuration.InterpreterPath));
}

private void Interpreter_ModuleNamesChanged(object sender, EventArgs e) {
Analyzer.Modules.ReInit();
foreach (var entry in Analyzer.ModulesByFilename) {
AnalysisQueue.Enqueue(entry.Value.ProjectEntry, AnalysisPriority.Normal);
}
}

private T ActivateObject<T>(string assemblyName, string typeName, Dictionary<string, object> properties) {
if (string.IsNullOrEmpty(assemblyName) || string.IsNullOrEmpty(typeName)) {
return default(T);
Expand Down