Fix wait for analysis complete, fix tests and avoid reload on all config changes #110
Conversation
| var package = state.AddModule("fob", "from .y import abc", "fob\\__init__.py"); | ||
| var x = state.AddModule("fob.x", "from .y import abc"); | ||
| var y = state.AddModule("fob.y", "abc = 42"); | ||
| /* |
There was a problem hiding this comment.
We don't need this offset.
| @@ -712,5 +703,39 @@ private sealed class ReloadModulesQueueItem : IAnalyzable { | |||
| return false; | |||
| } | |||
| #endregion | |||
|
|
|||
| internal Task WaitForCompleteAnalysisAsync(CancellationToken cancellationToken) { | |||
| var tcs = new TaskCompletionSource<object>(); | |||
There was a problem hiding this comment.
=> Task.WhenAny(WaitForCompleteAnalysisWorker(cancellationToken), Task.Delay(Timeout.Infinite, cancellationToken)).Unwrap();| @@ -83,6 +87,7 @@ public sealed partial class LanguageServer: IDisposable { | |||
| } | |||
|
|
|||
| public void Dispose() { | |||
| _shutdownCts.Cancel(); | |||
There was a problem hiding this comment.
_shutdownCts can be added to _disposables
| @@ -157,9 +157,13 @@ private class ActivePythonParse : IPythonParse { | |||
|
|
|||
| internal void SetCompleteAnalysis() { | |||
| lock (this) { | |||
| _analysisTcs.TrySetResult(Analysis); | |||
| if (AnalysisVersion == _expectedParse) { | |||
There was a problem hiding this comment.
this can be simplified a bit:
lock (this) {
if (AnalysisVersion != _expectedParse) {
return;
}
_analysisTcs.TrySetResult(Analysis);
}
RaiseNewAnalysis(); | } | ||
| OnDocumentChangeProcessingComplete(doc, t.Result as VersionCookie, enqueueForAnalysis, priority, p); | ||
| }).DoNotWait(); | ||
| await OnDocumentChangeProcessingCompleteAsync(doc, cookie as VersionCookie, enqueueForAnalysis, priority, p); |
There was a problem hiding this comment.
Before the change, EnqueueItem has been completed before ParseQueue.EnqueueAsync was completed. Now it waits for the parse to finish. Also, before the change exceptions from ParseQueue.Enqueue were logged but didn't affect the caller. Right now, exception will be propagated up to the DidChangeTextDocument.
There was a problem hiding this comment.
I can make it about the same so we don't change logic too much. What concerns me is that pending disposable in finally. Would it be disposed before cookieTask actually starts?
There was a problem hiding this comment.
Maybe it should be
catch {
pending?.Dispose();
throw
}| } | ||
| } | ||
|
|
||
| // The call must be fire and forget, but should not be yielding. | ||
| // It is called from DidChangeTextDocument which must fully finish | ||
| // since otherwise Complete() may come before the change is enqueued | ||
| // for processing and the completion list will be driven off the stale data. | ||
| var p = pending; | ||
| cookieTask.ContinueWith(t => { | ||
| return cookieTask.ContinueWith(async t => { |
There was a problem hiding this comment.
It looks like we should do
cookieTask.ContinueWith(...).DoNotWait();
return cookieTask;But if it is working as it is, let's leave it as it is :)
| @@ -100,5 +101,10 @@ internal sealed class ProjectFiles : IDisposable { | |||
| throw new ObjectDisposedException(nameof(ProjectFiles)); | |||
| } | |||
| } | |||
|
|
|||
| #region IEnumerable<IProjectEntry> | |||
| public IEnumerator<IProjectEntry> GetEnumerator() => All.GetEnumerator(); | |||
There was a problem hiding this comment.
Do we use it anywhere?
There was a problem hiding this comment.
Used to be foreach (var pf in ProjectFiles.All), enumerabe makes it simpler
| @@ -121,6 +122,7 @@ internal sealed class EditorFile { | |||
| }) | |||
| )); | |||
|
|
|||
| cancellationToken.ThrowIfCancellationRequested(); | |||
There was a problem hiding this comment.
Maybe check it before doc.UpdateDocument?
Fix wait for analysis complete, fix tests and avoid reload on all config changes
Fixes #111
Improves Reloading modules leaks memory #109 - avoid reloading modules on any configuration change. Still need to debug why reloading modules leaks.
Improves test stability as well as Rename/FindReferences/DocumentSymbols/GotoDefinition by waiting for the complete analysis.