From 68cdb04592d040537171eeb296bda6d78a0bc54d Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Tue, 22 Feb 2022 22:02:23 +1300 Subject: [PATCH 01/15] Initial work on AsyncAnalyzerWorkQueue --- .../Diagnostics/AsyncAnalyzerWorkQueue.cs | 263 ++++++++++++++++++ .../CSharpDiagnosticWorkerWithAnalyzers.cs | 179 ++++++------ 2 files changed, 342 insertions(+), 100 deletions(-) create mode 100644 src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs new file mode 100644 index 0000000000..f5d0b4d248 --- /dev/null +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -0,0 +1,263 @@ +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.Extensions.Logging; + +#nullable enable + +namespace OmniSharp.Roslyn.CSharp.Workers.Diagnostics +{ + public class AsyncAnalyzerWorkQueue + { + private readonly object _lock = new(); + private readonly Queue _forground = new(); + private readonly Queue _background = new(); + private readonly ILogger _logger; + private TaskCompletionSource _takeWorkWaiter = new(TaskCreationOptions.RunContinuationsAsynchronously); + private TaskCompletionSource _waitForgroundWaiter = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public AsyncAnalyzerWorkQueue(ILoggerFactory loggerFactory) + { + _waitForgroundWaiter.SetResult(null); + + _logger = loggerFactory.CreateLogger(); + } + + public void PutWork(IReadOnlyCollection documentIds, AnalyzerWorkType workType) + { + lock (_lock) + { + foreach (var documentId in documentIds) + { + _forground.RequestCancellationIfActive(documentId); + _background.RequestCancellationIfActive(documentId); + + if (workType == AnalyzerWorkType.Foreground) + { + if (_waitForgroundWaiter.Task.IsCompleted) + _waitForgroundWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + _forground.Enqueue(documentId); + + _background.Remove(documentId); + } + else if (workType == AnalyzerWorkType.Background) + { + if (_forground.IsEnqueued(documentId)) + continue; + + _background.Enqueue(documentId); + } + } + + if (!_takeWorkWaiter.Task.IsCompleted) + _takeWorkWaiter.SetResult(null); + } + } + + public async Task TakeWorkAsync() + { + while (true) + { + Task awaitTask; + + lock (_lock) + { + if (_forground.TryDequeue(out var documentId, out var cancellationTokenSource)) + { + return new QueueItem + ( + DocumentId: documentId, + CancellationToken: cancellationTokenSource.Token, + AnalyzerWorkType: AnalyzerWorkType.Foreground, + DocumentCount: _forground.MaximumPendingCount, + DocumentCountRemaining: _forground.PendingCount + ); + } + else if (_background.TryDequeue(out documentId, out cancellationTokenSource)) + { + return new QueueItem + ( + DocumentId: documentId, + CancellationToken: cancellationTokenSource.Token, + AnalyzerWorkType: AnalyzerWorkType.Background, + DocumentCount: _background.MaximumPendingCount, + DocumentCountRemaining: _background.PendingCount + ); + } + + if (_forground.PendingCount == 0 && _background.PendingCount == 0 && _takeWorkWaiter.Task.IsCompleted) + _takeWorkWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + awaitTask = _takeWorkWaiter.Task; + } + + await awaitTask; + } + } + + public void WorkComplete(QueueItem item) + { + lock (_lock) + { + if (item.AnalyzerWorkType == AnalyzerWorkType.Foreground) + { + _forground.WorkComplete(item.DocumentId); + + if (_forground.PendingCount == 0 + && _forground.ActiveCount == 0 + && !_waitForgroundWaiter.Task.IsCompleted) + { + _waitForgroundWaiter.SetResult(null); + } + } + else if (item.AnalyzerWorkType == AnalyzerWorkType.Background) + { + _background.WorkComplete(item.DocumentId); + } + } + } + + public async Task WaitForegroundWorkComplete(int? timeoutForPendingWorkMs = null) + { + var waitForgroundTask = _waitForgroundWaiter.Task; + + if (waitForgroundTask.IsCompleted) + return; + + if (timeoutForPendingWorkMs == null) + { + await waitForgroundTask; + + return; + } + + using var cancellationTokenSource = new CancellationTokenSource(); + + await Task.WhenAny( + Task.Delay(timeoutForPendingWorkMs.Value, cancellationTokenSource.Token), + waitForgroundTask); + + cancellationTokenSource.Cancel(); + + if (!waitForgroundTask.IsCompleted) + _logger.LogWarning($"Timeout before work got ready for foreground analysis queue. This is assertion to prevent complete api hang in case of error."); + } + + public bool TryPromote(DocumentId id) + { + var shouldEnqueue = false; + + lock (_lock) + { + shouldEnqueue = _forground.IsEnqueued(id) || _forground.IsActive(id); + } + + if (shouldEnqueue) + PutWork(new[] { id }, AnalyzerWorkType.Foreground); + + return shouldEnqueue; + } + + public record QueueItem + ( + DocumentId DocumentId, + CancellationToken CancellationToken, + AnalyzerWorkType AnalyzerWorkType, + int DocumentCount, + int DocumentCountRemaining + ); + + private class Queue + { + private readonly HashSet _hash = new(); + private readonly Queue _pending = new(); + private readonly Dictionary _active = new(); + + public int PendingCount => _pending.Count; + + public int ActiveCount => _active.Count; + + public int MaximumPendingCount { get; private set; } + + public void RequestCancellationIfActive(DocumentId documentId) + { + if (_active.TryGetValue(documentId, out var active)) + active.Cancel(); + } + + public void Enqueue(DocumentId documentId) + { + if (_hash.Add(documentId)) + { + _pending.Enqueue(documentId); + + if (_pending.Count > MaximumPendingCount) + MaximumPendingCount = _pending.Count; + } + } + + public bool IsEnqueued(DocumentId documentId) => + _hash.Contains(documentId); + + public bool IsActive(DocumentId documentId) => + _active.ContainsKey(documentId); + + public void Remove(DocumentId documentId) + { + if (_hash.Contains(documentId)) + { + _hash.Remove(documentId); + + var backgroundQueueItems = _pending.ToList(); + + _pending.Clear(); + + foreach (var item in backgroundQueueItems) + { + if (item != documentId) + _pending.Enqueue(item); + } + } + } + + public bool TryDequeue([NotNullWhen(true)] out DocumentId? documentId, [NotNullWhen(true)] out CancellationTokenSource? cancellationTokenSource) + { + if (_pending.Count > 0) + { + documentId = _pending.Dequeue(); + + _hash.Remove(documentId); + + _active[documentId] + = cancellationTokenSource + = new CancellationTokenSource(); + + return true; + } + + documentId = null; + cancellationTokenSource = null; + + return false; + } + + public bool WorkComplete(DocumentId documentId) + { + if (_active.TryGetValue(documentId, out var cancellationTokenSource)) + { + _active.Remove(documentId); + + cancellationTokenSource.Dispose(); + + return true; + } + + return false; + } + } + } +} diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 7b34d29a01..069e57646b 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -21,8 +21,7 @@ namespace OmniSharp.Roslyn.CSharp.Services.Diagnostics { public class CSharpDiagnosticWorkerWithAnalyzers : ICsDiagnosticWorker, IDisposable { - private readonly AnalyzerWorkQueue _workQueue; - private readonly SemaphoreSlim _throttler; + private readonly AsyncAnalyzerWorkQueue _workQueue; private readonly ILogger _logger; private readonly ConcurrentDictionary _currentDiagnosticResultLookup = new(); @@ -30,6 +29,7 @@ public class CSharpDiagnosticWorkerWithAnalyzers : ICsDiagnosticWorker, IDisposa private readonly DiagnosticEventForwarder _forwarder; private readonly OmniSharpOptions _options; private readonly OmniSharpWorkspace _workspace; + private readonly ImmutableArray _workerTasks; // This is workaround. // Currently roslyn doesn't expose official way to use IDE analyzers during analysis. @@ -45,8 +45,7 @@ public CSharpDiagnosticWorkerWithAnalyzers( { _logger = loggerFactory.CreateLogger(); _providers = providers.ToImmutableArray(); - _workQueue = new AnalyzerWorkQueue(loggerFactory, timeoutForPendingWorkMs: options.RoslynExtensionsOptions.DocumentAnalysisTimeoutMs * 3); - _throttler = new SemaphoreSlim(options.RoslynExtensionsOptions.DiagnosticWorkersThreadCount); + _workQueue = new AsyncAnalyzerWorkQueue(loggerFactory); _forwarder = forwarder; _options = options; @@ -61,8 +60,9 @@ public CSharpDiagnosticWorkerWithAnalyzers( _workspace.WorkspaceChanged += OnWorkspaceChanged; _workspace.OnInitialized += OnWorkspaceInitialized; - Task.Factory.StartNew(() => Worker(AnalyzerWorkType.Foreground), TaskCreationOptions.LongRunning); - Task.Factory.StartNew(() => Worker(AnalyzerWorkType.Background), TaskCreationOptions.LongRunning); + _workerTasks = Enumerable.Range(0, options.RoslynExtensionsOptions.DiagnosticWorkersThreadCount) + .Select(_ => Task.Run(Worker)) + .ToImmutableArray(); OnWorkspaceInitialized(_workspace.Initialized); } @@ -92,7 +92,7 @@ private async Task> GetDiagnosticsByDocument _workQueue.TryPromote(documentId); } - await _workQueue.WaitForegroundWorkComplete(); + await _workQueue.WaitForegroundWorkComplete(_options.RoslynExtensionsOptions.DocumentAnalysisTimeoutMs * 3); } return documentIds @@ -109,69 +109,53 @@ private ImmutableArray GetDocumentIdsFromPaths(ImmutableArray (projectId: solution.GetDocument(documentId)?.Project?.Id, documentId)) - .Where(x => x.projectId != null) - .ToImmutableArray(); - - var documentCount = documents.Length; - var documentCountRemaining = documentCount; + item = await _workQueue.TakeWorkAsync(); + var (documentId, cancellationToken, workType, documentCount, remaining) = item; - // event every percentage increase, or every 10th if there are fewer than 1000 - var eventEvery = Math.Max(10, documentCount / 100); - - var documentsGroupedByProjects = documents - .GroupBy(x => x.projectId, x => x.documentId) - .ToImmutableArray(); - var projectCount = documentsGroupedByProjects.Length; + if (workType == AnalyzerWorkType.Background) + { + // event every percentage increase, or every 10th if there are fewer than 1000 + var eventEvery = Math.Max(10, documentCount / 100); - EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Started, projectCount, documentCount, documentCountRemaining); + if (documentCount == remaining + 1) + EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Started, _projectCount, documentCount, remaining); - void decrementDocumentCountRemaining() - { - var remaining = Interlocked.Decrement(ref documentCountRemaining); var done = documentCount - remaining; - if (done % eventEvery == 0) - { - EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Progress, projectCount, documentCount, remaining); - } + if (done % eventEvery == 0 || remaining == 0) + EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Progress, _projectCount, documentCount, remaining); } + var solution = _workspace.CurrentSolution; + var projectId = solution.GetDocument(documentId)?.Project?.Id; + try { - var projectAnalyzerTasks = - documentsGroupedByProjects - .Select(projectGroup => Task.Run(async () => - { - var projectPath = solution.GetProject(projectGroup.Key).FilePath; - await AnalyzeProject(solution, projectGroup, decrementDocumentCountRemaining); - })) - .ToImmutableArray(); - - await Task.WhenAll(projectAnalyzerTasks); + if (projectId != null) + await AnalyzeDocument(solution, projectId, documentId); } finally { - EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Finished, projectCount, documentCount, documentCountRemaining); + if (remaining == 0) + EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Finished, _projectCount, documentCount, remaining); } - - _workQueue.WorkComplete(workType); - - await Task.Delay(50); } catch (Exception ex) { _logger.LogError($"Analyzer worker failed: {ex}"); } + finally + { + if (item != null) + _workQueue.WorkComplete(item); + } } } @@ -181,8 +165,17 @@ private void EventIfBackgroundWork(AnalyzerWorkType workType, BackgroundDiagnost _forwarder.BackgroundDiagnosticsStatus(status, numberProjects, numberFiles, numberFilesRemaining); } + private int _projectCount = 0; + private void QueueForAnalysis(ImmutableArray documentIds, AnalyzerWorkType workType) { + if (workType == AnalyzerWorkType.Background) + { + var solution = _workspace.CurrentSolution; + + _projectCount = documentIds.Select(x => solution.GetDocument(x)?.Project?.Id).Distinct().Count(x => x != null); + } + _workQueue.PutWork(documentIds, workType); } @@ -231,72 +224,55 @@ public async Task> AnalyzeDocumentAsync(Document documen return await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); } - public async Task> AnalyzeProjectsAsync(Project project, CancellationToken cancellationToken) + public Task> AnalyzeProjectsAsync(Project project, CancellationToken cancellationToken) { - var allAnalyzers = GetAnalyzersForProject(project); - var compilation = await project.GetCompilationAsync(cancellationToken); - var workspaceAnalyzerOptions = (AnalyzerOptions)_workspaceAnalyzerOptionsConstructor.Invoke(new object[] { project.AnalyzerOptions, project.Solution }); - var documentAnalyzerTasks = new List(); - var diagnostics = ImmutableList.Empty; - - foreach (var document in project.Documents) - { - await _throttler.WaitAsync(cancellationToken); - - documentAnalyzerTasks.Add(Task.Run(async () => - { - try - { - var documentDiagnostics = await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); - ImmutableInterlocked.Update(ref diagnostics, currentDiagnostics => currentDiagnostics.AddRange(documentDiagnostics)); - } - finally - { - _throttler.Release(); - } - }, cancellationToken)); - } - - await Task.WhenAll(documentAnalyzerTasks); - - return diagnostics; + throw new NotImplementedException(); + //var allAnalyzers = GetAnalyzersForProject(project); + //var compilation = await project.GetCompilationAsync(cancellationToken); + //var workspaceAnalyzerOptions = (AnalyzerOptions)_workspaceAnalyzerOptionsConstructor.Invoke(new object[] { project.AnalyzerOptions, project.Solution }); + //var documentAnalyzerTasks = new List(); + //var diagnostics = ImmutableList.Empty; + + //foreach (var document in project.Documents) + //{ + // await _throttler.WaitAsync(cancellationToken); + + // documentAnalyzerTasks.Add(Task.Run(async () => + // { + // try + // { + // var documentDiagnostics = await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); + // ImmutableInterlocked.Update(ref diagnostics, currentDiagnostics => currentDiagnostics.AddRange(documentDiagnostics)); + // } + // finally + // { + // _throttler.Release(); + // } + // }, cancellationToken)); + //} + + //await Task.WhenAll(documentAnalyzerTasks); + + //return diagnostics; } - private async Task AnalyzeProject(Solution solution, IGrouping documentsGroupedByProject, Action decrementRemaining) + private async Task AnalyzeDocument(Solution solution, ProjectId projectId, DocumentId documentId) { try { - var project = solution.GetProject(documentsGroupedByProject.Key); + var project = solution.GetProject(projectId); var allAnalyzers = GetAnalyzersForProject(project); var compilation = await project.GetCompilationAsync(); var workspaceAnalyzerOptions = (AnalyzerOptions)_workspaceAnalyzerOptionsConstructor.Invoke(new object[] { project.AnalyzerOptions, project.Solution }); - var documentAnalyzerTasks = new List(); + var document = project.GetDocument(documentId); - foreach (var documentId in documentsGroupedByProject) - { - await _throttler.WaitAsync(); + var diagnostics = await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); - documentAnalyzerTasks.Add(Task.Run(async () => - { - try - { - var document = project.GetDocument(documentId); - var diagnostics = await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); - UpdateCurrentDiagnostics(project, document, diagnostics); - decrementRemaining(); - } - finally - { - _throttler.Release(); - } - })); - } - - await Task.WhenAll(documentAnalyzerTasks); + UpdateCurrentDiagnostics(project, document, diagnostics); } catch (Exception ex) { - _logger.LogError($"Analysis of project {documentsGroupedByProject.Key} failed, underlaying error: {ex}"); + _logger.LogError($"Analysis of project {documentId} failed, underlaying error: {ex}"); } } @@ -415,6 +391,9 @@ public void Dispose() { _workspace.WorkspaceChanged -= OnWorkspaceChanged; _workspace.OnInitialized -= OnWorkspaceInitialized; + + // TODO: Clean up worker queue + // TODO: Clean up background threads } } } From a3a9a3d35e67743d264c4e8d62e33c53d68e55a5 Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Wed, 23 Feb 2022 12:42:58 +1300 Subject: [PATCH 02/15] Handle cancellations --- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 57 +++++++++++++------ 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 069e57646b..925dfe5780 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -114,11 +114,16 @@ private async Task Worker() while (true) { AsyncAnalyzerWorkQueue.QueueItem item = null; + DocumentId documentId; + CancellationToken? cancellationToken = null; + AnalyzerWorkType workType; + int documentCount; + int remaining; try { item = await _workQueue.TakeWorkAsync(); - var (documentId, cancellationToken, workType, documentCount, remaining) = item; + (documentId, cancellationToken, workType, documentCount, remaining) = item; if (workType == AnalyzerWorkType.Background) { @@ -139,7 +144,7 @@ private async Task Worker() try { if (projectId != null) - await AnalyzeDocument(solution, projectId, documentId); + await AnalyzeDocument(solution, projectId, documentId, cancellationToken.Value); } finally { @@ -147,6 +152,10 @@ private async Task Worker() EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Finished, _projectCount, documentCount, remaining); } } + catch (OperationCanceledException) when (cancellationToken != null && cancellationToken.Value.IsCancellationRequested) + { + _logger.LogInformation($"Analyzer work cancelled."); + } catch (Exception ex) { _logger.LogError($"Analyzer worker failed: {ex}"); @@ -220,8 +229,7 @@ public async Task> AnalyzeDocumentAsync(Document documen var compilation = await project.GetCompilationAsync(cancellationToken); var workspaceAnalyzerOptions = (AnalyzerOptions)_workspaceAnalyzerOptionsConstructor.Invoke(new object[] { project.AnalyzerOptions, project.Solution }); - cancellationToken.ThrowIfCancellationRequested(); - return await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); + return await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document, cancellationToken); } public Task> AnalyzeProjectsAsync(Project project, CancellationToken cancellationToken) @@ -256,8 +264,10 @@ public Task> AnalyzeProjectsAsync(Project project, Cance //return diagnostics; } - private async Task AnalyzeDocument(Solution solution, ProjectId projectId, DocumentId documentId) + private async Task AnalyzeDocument(Solution solution, ProjectId projectId, DocumentId documentId, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + try { var project = solution.GetProject(projectId); @@ -266,32 +276,39 @@ private async Task AnalyzeDocument(Solution solution, ProjectId projectId, Docum var workspaceAnalyzerOptions = (AnalyzerOptions)_workspaceAnalyzerOptionsConstructor.Invoke(new object[] { project.AnalyzerOptions, project.Solution }); var document = project.GetDocument(documentId); - var diagnostics = await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); + var diagnostics = await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document, cancellationToken); UpdateCurrentDiagnostics(project, document, diagnostics); } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; + } catch (Exception ex) { _logger.LogError($"Analysis of project {documentId} failed, underlaying error: {ex}"); } } - private async Task> AnalyzeDocument(Project project, ImmutableArray allAnalyzers, Compilation compilation, AnalyzerOptions workspaceAnalyzerOptions, Document document) + private async Task> AnalyzeDocument(Project project, ImmutableArray allAnalyzers, Compilation compilation, AnalyzerOptions workspaceAnalyzerOptions, Document document, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + + // There's real possibility that bug in analyzer causes analysis hang at document. + using var perDocumentTimeout = + new CancellationTokenSource(_options.RoslynExtensionsOptions.DocumentAnalysisTimeoutMs); + using var combinedCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, perDocumentTimeout.Token); + try { - // There's real possibility that bug in analyzer causes analysis hang at document. - var perDocumentTimeout = - new CancellationTokenSource(_options.RoslynExtensionsOptions.DocumentAnalysisTimeoutMs); - - var documentSemanticModel = await document.GetSemanticModelAsync(perDocumentTimeout.Token); + var documentSemanticModel = await document.GetSemanticModelAsync(combinedCancellation.Token); // Only basic syntax check is available if file is miscellanous like orphan .cs file. // Those projects are on hard coded virtual project if (project.Name == $"{Configuration.OmniSharpMiscProjectName}.csproj") { - var syntaxTree = await document.GetSyntaxTreeAsync(); - return syntaxTree.GetDiagnostics().ToImmutableArray(); + var syntaxTree = await document.GetSyntaxTreeAsync(combinedCancellation.Token); + return syntaxTree.GetDiagnostics(cancellationToken: combinedCancellation.Token).ToImmutableArray(); } else if (allAnalyzers.Any()) // Analyzers cannot be called with empty analyzer list. { @@ -303,22 +320,26 @@ private async Task> AnalyzeDocument(Project project, reportSuppressedDiagnostics: false)); var semanticDiagnosticsWithAnalyzers = await compilationWithAnalyzers - .GetAnalyzerSemanticDiagnosticsAsync(documentSemanticModel, filterSpan: null, perDocumentTimeout.Token); + .GetAnalyzerSemanticDiagnosticsAsync(documentSemanticModel, filterSpan: null, combinedCancellation.Token); var syntaxDiagnosticsWithAnalyzers = await compilationWithAnalyzers - .GetAnalyzerSyntaxDiagnosticsAsync(documentSemanticModel.SyntaxTree, perDocumentTimeout.Token); + .GetAnalyzerSyntaxDiagnosticsAsync(documentSemanticModel.SyntaxTree, combinedCancellation.Token); return semanticDiagnosticsWithAnalyzers .Concat(syntaxDiagnosticsWithAnalyzers) .Where(d => !d.IsSuppressed) - .Concat(documentSemanticModel.GetDiagnostics()) + .Concat(documentSemanticModel.GetDiagnostics(cancellationToken: combinedCancellation.Token)) .ToImmutableArray(); } else { - return documentSemanticModel.GetDiagnostics(); + return documentSemanticModel.GetDiagnostics(cancellationToken: combinedCancellation.Token); } } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; + } catch (Exception ex) { _logger.LogError($"Analysis of document {document.Name} failed or cancelled by timeout: {ex.Message}, analysers: {string.Join(", ", allAnalyzers)}"); From 7378243f9b373da4c83254abfc171adbccf8ce5a Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Thu, 24 Feb 2022 09:57:04 +1300 Subject: [PATCH 03/15] Improve cancellation handling --- .../Diagnostics/AsyncAnalyzerWorkQueue.cs | 41 ++++++++++++------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs index f5d0b4d248..f31e4d03d2 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -105,7 +105,7 @@ public void WorkComplete(QueueItem item) { if (item.AnalyzerWorkType == AnalyzerWorkType.Foreground) { - _forground.WorkComplete(item.DocumentId); + _forground.WorkComplete(item.DocumentId, item.CancellationToken); if (_forground.PendingCount == 0 && _forground.ActiveCount == 0 @@ -116,7 +116,7 @@ public void WorkComplete(QueueItem item) } else if (item.AnalyzerWorkType == AnalyzerWorkType.Background) { - _background.WorkComplete(item.DocumentId); + _background.WorkComplete(item.DocumentId, item.CancellationToken); } } } @@ -175,7 +175,7 @@ private class Queue { private readonly HashSet _hash = new(); private readonly Queue _pending = new(); - private readonly Dictionary _active = new(); + private readonly Dictionary> _active = new(); public int PendingCount => _pending.Count; @@ -186,7 +186,10 @@ private class Queue public void RequestCancellationIfActive(DocumentId documentId) { if (_active.TryGetValue(documentId, out var active)) - active.Cancel(); + { + foreach (var cts in active) + cts.Cancel(); + } } public void Enqueue(DocumentId documentId) @@ -232,9 +235,12 @@ public bool TryDequeue([NotNullWhen(true)] out DocumentId? documentId, [NotNullW _hash.Remove(documentId); - _active[documentId] - = cancellationTokenSource - = new CancellationTokenSource(); + if (!_active.TryGetValue(documentId, out var cancellationTokenSources)) + _active[documentId] = cancellationTokenSources = new List(); + + cancellationTokenSource = new CancellationTokenSource(); + + cancellationTokenSources.Add(cancellationTokenSource); return true; } @@ -245,18 +251,25 @@ public bool TryDequeue([NotNullWhen(true)] out DocumentId? documentId, [NotNullW return false; } - public bool WorkComplete(DocumentId documentId) + public void WorkComplete(DocumentId documentId, CancellationToken cancellationToken) { - if (_active.TryGetValue(documentId, out var cancellationTokenSource)) + if (_active.TryGetValue(documentId, out var cancellationTokenSources)) { - _active.Remove(documentId); + foreach (var cancellationTokenSource in cancellationTokenSources.ToList()) + { + if (cancellationTokenSource.Token == cancellationToken) + { + cancellationTokenSource.Dispose(); - cancellationTokenSource.Dispose(); + cancellationTokenSources.Remove(cancellationTokenSource); - return true; - } + break; + } + } - return false; + if (cancellationTokenSources.Count == 0) + _active.Remove(documentId); + } } } } From 56c954cc1a2a2ce2523b5a3fb6ac1249442f355f Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Thu, 24 Feb 2022 18:24:26 +1300 Subject: [PATCH 04/15] Fix AnalyzeProjectsAsync --- .../Diagnostics/AsyncAnalyzerWorkQueue.cs | 21 +++++----- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 41 +++++-------------- 2 files changed, 21 insertions(+), 41 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs index f31e4d03d2..f7de85e34c 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -121,30 +121,29 @@ public void WorkComplete(QueueItem item) } } - public async Task WaitForegroundWorkComplete(int? timeoutForPendingWorkMs = null) + public async Task WaitForegroundWorkComplete(CancellationToken cancellationToken = default) { var waitForgroundTask = _waitForgroundWaiter.Task; - if (waitForgroundTask.IsCompleted) + if (waitForgroundTask.IsCompleted || cancellationToken.IsCancellationRequested) return; - if (timeoutForPendingWorkMs == null) + if (cancellationToken == default) { await waitForgroundTask; return; } - using var cancellationTokenSource = new CancellationTokenSource(); + var taskCompletion = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - await Task.WhenAny( - Task.Delay(timeoutForPendingWorkMs.Value, cancellationTokenSource.Token), - waitForgroundTask); - - cancellationTokenSource.Cancel(); + using (cancellationToken.Register(() => taskCompletion.SetResult(null))) + { + await Task.WhenAny(taskCompletion.Task, waitForgroundTask); - if (!waitForgroundTask.IsCompleted) - _logger.LogWarning($"Timeout before work got ready for foreground analysis queue. This is assertion to prevent complete api hang in case of error."); + if (!waitForgroundTask.IsCompleted) + _logger.LogWarning($"Timeout before work got ready for foreground analysis queue. This is assertion to prevent complete api hang in case of error."); + } } public bool TryPromote(DocumentId id) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 925dfe5780..c36041c119 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -92,7 +92,9 @@ private async Task> GetDiagnosticsByDocument _workQueue.TryPromote(documentId); } - await _workQueue.WaitForegroundWorkComplete(_options.RoslynExtensionsOptions.DocumentAnalysisTimeoutMs * 3); + using var cancellationTokenSource = new CancellationTokenSource(_options.RoslynExtensionsOptions.DocumentAnalysisTimeoutMs * 3); + + await _workQueue.WaitForegroundWorkComplete(cancellationTokenSource.Token); } return documentIds @@ -232,36 +234,15 @@ public async Task> AnalyzeDocumentAsync(Document documen return await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document, cancellationToken); } - public Task> AnalyzeProjectsAsync(Project project, CancellationToken cancellationToken) + public async Task> AnalyzeProjectsAsync(Project project, CancellationToken cancellationToken) { - throw new NotImplementedException(); - //var allAnalyzers = GetAnalyzersForProject(project); - //var compilation = await project.GetCompilationAsync(cancellationToken); - //var workspaceAnalyzerOptions = (AnalyzerOptions)_workspaceAnalyzerOptionsConstructor.Invoke(new object[] { project.AnalyzerOptions, project.Solution }); - //var documentAnalyzerTasks = new List(); - //var diagnostics = ImmutableList.Empty; - - //foreach (var document in project.Documents) - //{ - // await _throttler.WaitAsync(cancellationToken); - - // documentAnalyzerTasks.Add(Task.Run(async () => - // { - // try - // { - // var documentDiagnostics = await AnalyzeDocument(project, allAnalyzers, compilation, workspaceAnalyzerOptions, document); - // ImmutableInterlocked.Update(ref diagnostics, currentDiagnostics => currentDiagnostics.AddRange(documentDiagnostics)); - // } - // finally - // { - // _throttler.Release(); - // } - // }, cancellationToken)); - //} - - //await Task.WhenAll(documentAnalyzerTasks); - - //return diagnostics; + QueueForAnalysis(project.DocumentIds.ToImmutableArray(), AnalyzerWorkType.Foreground); + + await _workQueue.WaitForegroundWorkComplete(cancellationToken); + + return _currentDiagnosticResultLookup + .SelectMany(X => X.Value.Diagnostics) + .ToImmutableArray(); } private async Task AnalyzeDocument(Solution solution, ProjectId projectId, DocumentId documentId, CancellationToken cancellationToken) From 901f0c5973acc7a5be1812c29b0b419fac5c39a0 Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Sun, 27 Feb 2022 20:11:27 +1300 Subject: [PATCH 05/15] Added AnalyzerWorkQueue unit tests --- .../Diagnostics/AsyncAnalyzerWorkQueue.cs | 35 +- .../AsyncAnalyzerWorkerQueueFacts.cs | 333 ++++++++++++++++++ 2 files changed, 363 insertions(+), 5 deletions(-) create mode 100644 tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs index f7de85e34c..11e046f592 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -26,6 +26,15 @@ public AsyncAnalyzerWorkQueue(ILoggerFactory loggerFactory) _logger = loggerFactory.CreateLogger(); } + public int PendingCount + { + get + { + lock (_lock) + return _forground.PendingCount + _background.PendingCount; + } + } + public void PutWork(IReadOnlyCollection documentIds, AnalyzerWorkType workType) { lock (_lock) @@ -58,10 +67,12 @@ public void PutWork(IReadOnlyCollection documentIds, AnalyzerWorkTyp } } - public async Task TakeWorkAsync() + public async Task TakeWorkAsync(CancellationToken cancellationToken = default) { while (true) { + cancellationToken.ThrowIfCancellationRequested(); + Task awaitTask; lock (_lock) @@ -95,7 +106,21 @@ public async Task TakeWorkAsync() awaitTask = _takeWorkWaiter.Task; } - await awaitTask; + if (cancellationToken == default) + { + await awaitTask.ConfigureAwait(false); + } + else + { + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + using (cancellationToken.Register(() => tcs.SetResult(null))) + { + await Task.WhenAny(awaitTask, tcs.Task).ConfigureAwait(false); + } + + } + } } @@ -130,7 +155,7 @@ public async Task WaitForegroundWorkComplete(CancellationToken cancellationToken if (cancellationToken == default) { - await waitForgroundTask; + await waitForgroundTask.ConfigureAwait(false); return; } @@ -139,7 +164,7 @@ public async Task WaitForegroundWorkComplete(CancellationToken cancellationToken using (cancellationToken.Register(() => taskCompletion.SetResult(null))) { - await Task.WhenAny(taskCompletion.Task, waitForgroundTask); + await Task.WhenAny(taskCompletion.Task, waitForgroundTask).ConfigureAwait(false); if (!waitForgroundTask.IsCompleted) _logger.LogWarning($"Timeout before work got ready for foreground analysis queue. This is assertion to prevent complete api hang in case of error."); @@ -152,7 +177,7 @@ public bool TryPromote(DocumentId id) lock (_lock) { - shouldEnqueue = _forground.IsEnqueued(id) || _forground.IsActive(id); + shouldEnqueue = _background.IsEnqueued(id) || _background.IsActive(id); } if (shouldEnqueue) diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs new file mode 100644 index 0000000000..34cf5a4616 --- /dev/null +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs @@ -0,0 +1,333 @@ +using System; +using System.Collections.Immutable; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.Extensions.Logging; +using OmniSharp.Roslyn.CSharp.Workers.Diagnostics; +using Xunit; + +namespace OmniSharp.Roslyn.CSharp.Tests +{ +#pragma warning disable VSTHRD103 // Call async methods when in an async method + public class AsyncAnalyzerWorkerQueueFacts + { + private class Logger : ILogger + { + public IDisposable BeginScope(TState state) + { + return null; + } + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) + { + RecordedMessages = RecordedMessages.Add(state.ToString()); + } + + public ImmutableArray RecordedMessages { get; set; } = ImmutableArray.Create(); + } + + private class LoggerFactory : ILoggerFactory + { + public Logger Logger { get; } = new Logger(); + + public void AddProvider(ILoggerProvider provider) + { + } + + public ILogger CreateLogger(string categoryName) + { + return Logger; + } + + public void Dispose() + { + } + } + + [Theory] + [InlineData(AnalyzerWorkType.Background)] + [InlineData(AnalyzerWorkType.Foreground)] + public async Task WhenWorksIsAddedToQueueThenTheyWillBeReturned(AnalyzerWorkType workType) + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, workType); + + var work = await queue.TakeWorkAsyncWithTimeout(); + + Assert.Equal(document, work.DocumentId); + Assert.Equal(0, queue.PendingCount); + } + + [Fact] + public async Task WhenForegroundWorkIsAddedThenWaitNextIterationOfItReady() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); + + var pendingTask = queue.WaitForegroundWorkCompleteWithTimeout(500); + + Assert.False(pendingTask.IsCompleted); + + var work = await queue.TakeWorkAsyncWithTimeout(); + + queue.WorkComplete(work); + + pendingTask.Wait(TimeSpan.FromMilliseconds(50)); + + Assert.True(pendingTask.IsCompleted); + } + + [Fact] + public async Task WhenForegroundWorkIsUnderAnalysisOutFromQueueThenWaitUntilNextIterationOfItIsReady() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); + + var work = await queue.TakeWorkAsync(); + + var pendingTask = queue.WaitForegroundWorkCompleteWithTimeout(500); + pendingTask.Wait(TimeSpan.FromMilliseconds(50)); + + Assert.False(pendingTask.IsCompleted); + queue.WorkComplete(work); + pendingTask.Wait(TimeSpan.FromMilliseconds(50)); + Assert.True(pendingTask.IsCompleted); + } + + [Fact] + public async Task WhenMultipleThreadsAreConsumingAnalyzerWorkerQueueItWorksAsExpected() + { + var now = DateTime.UtcNow; + + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + + var parallelQueues = + Enumerable.Range(0, 10) + .Select(_ => + Task.Run(async () => + { + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); + + var work = await queue.TakeWorkAsync(); + + var pendingTask = queue.WaitForegroundWorkCompleteWithTimeout(1000); + + var pendingTask2 = queue.WaitForegroundWorkCompleteWithTimeout(1000); + + pendingTask.Wait(TimeSpan.FromMilliseconds(300)); + })) + .ToArray(); + + await Task.WhenAll(parallelQueues); + + Assert.Equal(0, queue.PendingCount); + } + + [Fact] + public async Task WhenWorkIsAddedAgainWhenPreviousIsAnalysing_ThenDontWaitAnotherOneToGetReady() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); + + var work = await queue.TakeWorkAsync(); + var waitingCall = Task.Run(async () => await queue.WaitForegroundWorkCompleteWithTimeout(10 * 1000)); + await Task.Delay(50); + + // User updates code -> document is queued again during period when theres already api call waiting + // to continue. + queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); + + // First iteration of work is done. + queue.WorkComplete(work); + + // Waiting call continues because its iteration of work is done, even when theres next + // already waiting. + waitingCall.Wait(50); + + Assert.True(waitingCall.IsCompleted); + } + + [Fact] + public void WhenBackgroundWorkIsAdded_DontWaitIt() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Background); + + Assert.True(queue.WaitForegroundWorkComplete().IsCompleted); + } + + [Fact] + public void WhenSingleFileIsPromoted_ThenPromoteItFromBackgroundQueueToForeground() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Background); + + queue.TryPromote(document); + + Assert.NotEqual(0, queue.PendingCount); + } + + [Fact] + public void WhenFileIsntAtBackgroundQueueAndTriedToBePromoted_ThenDontDoNothing() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.TryPromote(document); + + Assert.Equal(0, queue.PendingCount); + } + + [Fact] + public async Task WhenFileIsProcessingInBackgroundQueue_ThenPromoteItAsForeground() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Background); + + await queue.TakeWorkAsyncWithTimeout(); + + queue.TryPromote(document); + + await queue.TakeWorkAsyncWithTimeout(); + } + + [Fact] + public async Task WhenFileIsAddedMultipleTimes_DuplicatesAreIgnored() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Background); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Background); + + await queue.TakeWorkAsyncWithTimeout(); + + await Assert.ThrowsAsync(() => + queue.TakeWorkAsyncWithTimeout()); + } + + [Fact] + public async Task WhenFileIsAddedWhileProcessing_ThePeviousRunIsCancelled() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Background); + + var result = await queue.TakeWorkAsyncWithTimeout(); + + Assert.False(result.CancellationToken.IsCancellationRequested); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Background); + + Assert.True(result.CancellationToken.IsCancellationRequested); + } + + [Fact] + public async Task WhenQueueIsEmpty_TakeWorkRespondsToCancellation() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + await Assert.ThrowsAsync(() => + queue.TakeWorkAsyncWithTimeout()); + } + + [Fact] + public async Task WhenAwaitingForForgroundWork_CancellationIsHandled() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document = CreateTestDocumentId(); + + queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); + + await Assert.ThrowsAsync(() => + queue.WaitForegroundWorkCompleteWithTimeout(50)); + } + + [Fact] + public async Task WhenDequeingWork_ItsReturnedInOrderForgroundFirst() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document1 = CreateTestDocumentId(); + var document2 = CreateTestDocumentId(); + var document3 = CreateTestDocumentId(); + var document4 = CreateTestDocumentId(); + + queue.PutWork(new[] { document3 }, AnalyzerWorkType.Background); + + queue.PutWork(new[] { document1 }, AnalyzerWorkType.Foreground); + + queue.PutWork(new[] { document4 }, AnalyzerWorkType.Background); + + queue.PutWork(new[] { document2 }, AnalyzerWorkType.Foreground); + + var result1 = await queue.TakeWorkAsyncWithTimeout(); + + Assert.Equal(document1, result1.DocumentId); + + var result2 = await queue.TakeWorkAsyncWithTimeout(); + + Assert.Equal(document2, result2.DocumentId); + + var result3 = await queue.TakeWorkAsyncWithTimeout(); + + Assert.Equal(document3, result3.DocumentId); + + var result4 = await queue.TakeWorkAsyncWithTimeout(); + + Assert.Equal(document4, result4.DocumentId); + } + + private static DocumentId CreateTestDocumentId() + { + var projectInfo = ProjectInfo.Create( + id: ProjectId.CreateNewId(), + version: VersionStamp.Create(), + name: "testProject", + assemblyName: "AssemblyName", + language: LanguageNames.CSharp); + + return DocumentId.CreateNewId(projectInfo.Id); + } + } + + public static class AsyncAnalyzerWorkerQueueFactsExtensions + { + public static async Task TakeWorkAsyncWithTimeout(this AsyncAnalyzerWorkQueue queue) + { + using var cts = new CancellationTokenSource(50); + + return await queue.TakeWorkAsync(cts.Token); + } + + public static async Task WaitForegroundWorkCompleteWithTimeout(this AsyncAnalyzerWorkQueue queue, int timeout) + { + using var cts = new CancellationTokenSource(timeout); + + await queue.WaitForegroundWorkComplete(cts.Token); + } + } +#pragma warning restore VSTHRD103 // Call async methods when in an async method +} From c3d8a7d24f1688d983150af69da090cfd6eab093 Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Tue, 1 Mar 2022 20:25:18 +1300 Subject: [PATCH 06/15] Initial unit test fixes --- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 14 +++++++++----- .../AsyncAnalyzerWorkerQueueFacts.cs | 9 ++++++--- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index c36041c119..5b5d43a410 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -98,8 +98,8 @@ private async Task> GetDiagnosticsByDocument } return documentIds - .Where(x => _currentDiagnosticResultLookup.ContainsKey(x)) - .Select(x => _currentDiagnosticResultLookup[x]) + .Select(x => _currentDiagnosticResultLookup.TryGetValue(x, out var value) ? value : null) + .Where(x => x != null) .ToImmutableArray(); } @@ -236,12 +236,16 @@ public async Task> AnalyzeDocumentAsync(Document documen public async Task> AnalyzeProjectsAsync(Project project, CancellationToken cancellationToken) { - QueueForAnalysis(project.DocumentIds.ToImmutableArray(), AnalyzerWorkType.Foreground); + var documentIds = project.DocumentIds.ToImmutableArray(); + + QueueForAnalysis(documentIds, AnalyzerWorkType.Foreground); await _workQueue.WaitForegroundWorkComplete(cancellationToken); - return _currentDiagnosticResultLookup - .SelectMany(X => X.Value.Diagnostics) + return documentIds + .Select(x => _currentDiagnosticResultLookup.TryGetValue(x, out var value) ? value : null) + .Where(x => x != null) + .SelectMany(x => x.Diagnostics) .ToImmutableArray(); } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs index 34cf5a4616..a999e5e551 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs @@ -262,8 +262,9 @@ public async Task WhenAwaitingForForgroundWork_CancellationIsHandled() queue.PutWork(new[] { document }, AnalyzerWorkType.Foreground); - await Assert.ThrowsAsync(() => - queue.WaitForegroundWorkCompleteWithTimeout(50)); + var isCancelled = await queue.WaitForegroundWorkCompleteWithTimeout(50); + + Assert.True(isCancelled); } [Fact] @@ -322,11 +323,13 @@ public static class AsyncAnalyzerWorkerQueueFactsExtensions return await queue.TakeWorkAsync(cts.Token); } - public static async Task WaitForegroundWorkCompleteWithTimeout(this AsyncAnalyzerWorkQueue queue, int timeout) + public static async Task WaitForegroundWorkCompleteWithTimeout(this AsyncAnalyzerWorkQueue queue, int timeout) { using var cts = new CancellationTokenSource(timeout); await queue.WaitForegroundWorkComplete(cts.Token); + + return cts.Token.IsCancellationRequested; } } #pragma warning restore VSTHRD103 // Call async methods when in an async method From 4ce8860e86a6167def33aaead4b347ece0fa681b Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Tue, 1 Mar 2022 20:47:46 +1300 Subject: [PATCH 07/15] For forground waiting when addtional documents are added --- .../Diagnostics/AsyncAnalyzerWorkQueue.cs | 57 ++++++++++++------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs index 11e046f592..0c4e5f8137 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -17,12 +17,9 @@ public class AsyncAnalyzerWorkQueue private readonly Queue _background = new(); private readonly ILogger _logger; private TaskCompletionSource _takeWorkWaiter = new(TaskCreationOptions.RunContinuationsAsynchronously); - private TaskCompletionSource _waitForgroundWaiter = new(TaskCreationOptions.RunContinuationsAsynchronously); public AsyncAnalyzerWorkQueue(ILoggerFactory loggerFactory) { - _waitForgroundWaiter.SetResult(null); - _logger = loggerFactory.CreateLogger(); } @@ -46,9 +43,6 @@ public void PutWork(IReadOnlyCollection documentIds, AnalyzerWorkTyp if (workType == AnalyzerWorkType.Foreground) { - if (_waitForgroundWaiter.Task.IsCompleted) - _waitForgroundWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - _forground.Enqueue(documentId); _background.Remove(documentId); @@ -129,28 +123,23 @@ public void WorkComplete(QueueItem item) lock (_lock) { if (item.AnalyzerWorkType == AnalyzerWorkType.Foreground) - { _forground.WorkComplete(item.DocumentId, item.CancellationToken); - - if (_forground.PendingCount == 0 - && _forground.ActiveCount == 0 - && !_waitForgroundWaiter.Task.IsCompleted) - { - _waitForgroundWaiter.SetResult(null); - } - } else if (item.AnalyzerWorkType == AnalyzerWorkType.Background) - { _background.WorkComplete(item.DocumentId, item.CancellationToken); - } } } public async Task WaitForegroundWorkComplete(CancellationToken cancellationToken = default) { - var waitForgroundTask = _waitForgroundWaiter.Task; + if (cancellationToken.IsCancellationRequested) + return; + + Task waitForgroundTask; - if (waitForgroundTask.IsCompleted || cancellationToken.IsCancellationRequested) + lock (_lock) + waitForgroundTask = _forground.GetWaiter(); + + if (waitForgroundTask.IsCompleted) return; if (cancellationToken == default) @@ -200,6 +189,7 @@ private class Queue private readonly HashSet _hash = new(); private readonly Queue _pending = new(); private readonly Dictionary> _active = new(); + private readonly List<(HashSet DocumentIds, TaskCompletionSource TaskCompletionSource)> _waiters = new(); public int PendingCount => _pending.Count; @@ -293,7 +283,36 @@ public void WorkComplete(DocumentId documentId, CancellationToken cancellationTo if (cancellationTokenSources.Count == 0) _active.Remove(documentId); + + foreach (var waiter in _waiters.ToList()) + { + if (waiter.DocumentIds.Remove(documentId) && waiter.DocumentIds.Count == 0) + { + waiter.TaskCompletionSource.SetResult(null); + + _waiters.Remove(waiter); + } + } + } + } + + public Task GetWaiter() + { + if (_active.Count == 0 && _pending.Count == 0) + return Task.CompletedTask; + + var documentIds = new HashSet(_hash.Concat(_active.Keys)); + + var waiter = _waiters.FirstOrDefault(x => x.DocumentIds.SetEquals(documentIds)); + + if (waiter == default) + { + waiter = (documentIds, new TaskCompletionSource()); + + _waiters.Add(waiter); } + + return waiter.TaskCompletionSource.Task; } } } From 8cb7d1505ea4f37f1f4e014081e6f3989361fe53 Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Thu, 3 Mar 2022 16:31:57 +1300 Subject: [PATCH 08/15] Remove comments --- .../Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 5b5d43a410..43e0293141 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -397,9 +397,6 @@ public void Dispose() { _workspace.WorkspaceChanged -= OnWorkspaceChanged; _workspace.OnInitialized -= OnWorkspaceInitialized; - - // TODO: Clean up worker queue - // TODO: Clean up background threads } } } From cae6e9eff6722d67c2214270fd27d0c016e6059c Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Thu, 3 Mar 2022 16:57:30 +1300 Subject: [PATCH 09/15] Additonal tidy up --- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 43e0293141..e3b544dfca 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -29,7 +29,9 @@ public class CSharpDiagnosticWorkerWithAnalyzers : ICsDiagnosticWorker, IDisposa private readonly DiagnosticEventForwarder _forwarder; private readonly OmniSharpOptions _options; private readonly OmniSharpWorkspace _workspace; - private readonly ImmutableArray _workerTasks; + + private int _projectCount = 0; + // This is workaround. // Currently roslyn doesn't expose official way to use IDE analyzers during analysis. @@ -60,9 +62,8 @@ public CSharpDiagnosticWorkerWithAnalyzers( _workspace.WorkspaceChanged += OnWorkspaceChanged; _workspace.OnInitialized += OnWorkspaceInitialized; - _workerTasks = Enumerable.Range(0, options.RoslynExtensionsOptions.DiagnosticWorkersThreadCount) - .Select(_ => Task.Run(Worker)) - .ToImmutableArray(); + for (var i = 0; i < options.RoslynExtensionsOptions.DiagnosticWorkersThreadCount; i++) + Task.Run(Worker); OnWorkspaceInitialized(_workspace.Initialized); } @@ -176,8 +177,6 @@ private void EventIfBackgroundWork(AnalyzerWorkType workType, BackgroundDiagnost _forwarder.BackgroundDiagnosticsStatus(status, numberProjects, numberFiles, numberFilesRemaining); } - private int _projectCount = 0; - private void QueueForAnalysis(ImmutableArray documentIds, AnalyzerWorkType workType) { if (workType == AnalyzerWorkType.Background) @@ -271,7 +270,7 @@ private async Task AnalyzeDocument(Solution solution, ProjectId projectId, Docum } catch (Exception ex) { - _logger.LogError($"Analysis of project {documentId} failed, underlaying error: {ex}"); + _logger.LogError($"Analysis of document {documentId} failed, underlying error: {ex}"); } } From a7deebabda95544183eb8b6f45140614e74ef050 Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Sat, 5 Mar 2022 08:29:53 +1300 Subject: [PATCH 10/15] Unit test fixes --- .../Diagnostics/AsyncAnalyzerWorkQueue.cs | 16 +++++++--- .../AsyncAnalyzerWorkerQueueFacts.cs | 31 +++++++++++++++++-- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs index 0c4e5f8137..808e2984d0 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -284,15 +284,21 @@ public void WorkComplete(DocumentId documentId, CancellationToken cancellationTo if (cancellationTokenSources.Count == 0) _active.Remove(documentId); - foreach (var waiter in _waiters.ToList()) + var isReenqueued = cancellationToken.IsCancellationRequested + && (_hash.Contains(documentId) || _active.ContainsKey(documentId)); + + if (!isReenqueued) { - if (waiter.DocumentIds.Remove(documentId) && waiter.DocumentIds.Count == 0) + foreach (var waiter in _waiters.ToList()) { - waiter.TaskCompletionSource.SetResult(null); + if (waiter.DocumentIds.Remove(documentId) && waiter.DocumentIds.Count == 0) + { + waiter.TaskCompletionSource.SetResult(null); - _waiters.Remove(waiter); + _waiters.Remove(waiter); + } } - } + } } } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs index a999e5e551..53acbc2734 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs @@ -136,7 +136,34 @@ public async Task WhenMultipleThreadsAreConsumingAnalyzerWorkerQueueItWorksAsExp } [Fact] - public async Task WhenWorkIsAddedAgainWhenPreviousIsAnalysing_ThenDontWaitAnotherOneToGetReady() + public async Task WheNewnWorkIsAddedAgainWhenPreviousIsAnalysing_ThenDontWaitAnotherOneToGetReady() + { + var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); + var document1 = CreateTestDocumentId(); + var document2 = CreateTestDocumentId(); + + queue.PutWork(new[] { document1 }, AnalyzerWorkType.Foreground); + + var work = await queue.TakeWorkAsync(); + var waitingCall = Task.Run(async () => await queue.WaitForegroundWorkCompleteWithTimeout(10 * 1000)); + await Task.Delay(50); + + // User updates code -> document is queued again during period when theres already api call waiting + // to continue. + queue.PutWork(new[] { document2 }, AnalyzerWorkType.Foreground); + + // First iteration of work is done. + queue.WorkComplete(work); + + // Waiting call continues because its iteration of work is done, even when theres next + // already waiting. + waitingCall.Wait(50); + + Assert.True(waitingCall.IsCompleted); + } + + [Fact] + public async Task WhenWorkIsAddedAgainWhenPreviousIsAnalysing_ThenContinueWaiting() { var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); var document = CreateTestDocumentId(); @@ -158,7 +185,7 @@ public async Task WhenWorkIsAddedAgainWhenPreviousIsAnalysing_ThenDontWaitAnothe // already waiting. waitingCall.Wait(50); - Assert.True(waitingCall.IsCompleted); + Assert.False(waitingCall.IsCompleted); } [Fact] From 27cc4580fe9a0bba4c6b31cb06a46c0baa071ff6 Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Sat, 5 Mar 2022 12:55:06 +1300 Subject: [PATCH 11/15] Fix concurrency of TestEventEmitter --- .../TestEventEmitter.cs | 70 +++++++++++++------ 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/TestEventEmitter.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/TestEventEmitter.cs index da0e41afc2..ba5fa9d46c 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/TestEventEmitter.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/TestEventEmitter.cs @@ -1,46 +1,72 @@ using System; -using System.Collections.Concurrent; -using System.Collections.Immutable; +using System.Collections.Generic; using System.Linq; using System.Linq.Expressions; +using System.Threading; using System.Threading.Tasks; using OmniSharp.Eventing; namespace OmniSharp.Roslyn.CSharp.Tests { public class TestEventEmitter : IEventEmitter + { + private readonly object _lock = new(); + private readonly List _messages = new(); + private readonly List<(Predicate Predicate, TaskCompletionSource TaskCompletionSource)> _predicates = new(); + + public async Task ExpectForEmitted(Expression> predicate) { - public ImmutableArray Messages { get; private set; } = ImmutableArray.Empty; + var asCompiledPredicate = predicate.Compile(); + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - public async Task ExpectForEmitted(Expression> predicate) + lock (_lock) { - var asCompiledPredicate = predicate.Compile(); + if (_messages.Any(m => asCompiledPredicate(m))) + return; - // May seem hacky but nothing is more painfull to debug than infinite hanging test ... - for(int i = 0; i < 100; i++) - { - if(Messages.Any(m => asCompiledPredicate(m))) - { - return; - } + _predicates.Add((asCompiledPredicate, tcs)); + } - await Task.Delay(250); - } + try + { + using var cts = new CancellationTokenSource(25000); - throw new InvalidOperationException($"Timeout reached before expected event count reached before prediction {predicate} came true, current diagnostics '{String.Join(";", Messages)}'"); - } + cts.Token.Register(() => tcs.SetCanceled()); - public void Clear() + await tcs.Task; + } + catch (OperationCanceledException) { - Messages = ImmutableArray.Empty; + throw new InvalidOperationException($"Timeout reached before expected event count reached before prediction {predicate} came true, current diagnostics '{String.Join(";", _messages)}'"); } + finally + { + lock (_lock) + _predicates.Remove((asCompiledPredicate, tcs)); + } + } + + public void Clear() + { + lock (_lock) + _messages.Clear(); + } - public void Emit(string kind, object args) + public void Emit(string kind, object args) + { + if (args is T asT) { - if(args is T asT) + lock (_lock) { - Messages = Messages.Add(asT); + _messages.Add(asT); + + foreach (var (predicate, tcs) in _predicates) + { + if (predicate(asT)) + tcs.SetResult(null); + } } } } -} \ No newline at end of file + } +} From 13889ec5d6928adb2edbac4415c2e69c348136aa Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Sat, 5 Mar 2022 20:41:55 +1300 Subject: [PATCH 12/15] Tweak async worker queue to not switch items between forground and background queues --- .../Diagnostics/AsyncAnalyzerWorkQueue.cs | 47 ++++++++----------- .../TestEventEmitter.cs | 5 +- 2 files changed, 23 insertions(+), 29 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs index 808e2984d0..457065ee8d 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -42,18 +42,9 @@ public void PutWork(IReadOnlyCollection documentIds, AnalyzerWorkTyp _background.RequestCancellationIfActive(documentId); if (workType == AnalyzerWorkType.Foreground) - { _forground.Enqueue(documentId); - - _background.Remove(documentId); - } else if (workType == AnalyzerWorkType.Background) - { - if (_forground.IsEnqueued(documentId)) - continue; - _background.Enqueue(documentId); - } } if (!_takeWorkWaiter.Task.IsCompleted) @@ -186,12 +177,12 @@ int DocumentCountRemaining private class Queue { - private readonly HashSet _hash = new(); - private readonly Queue _pending = new(); + private readonly HashSet _pendingHash = new(); + private readonly Queue _pendingQueue = new(); private readonly Dictionary> _active = new(); private readonly List<(HashSet DocumentIds, TaskCompletionSource TaskCompletionSource)> _waiters = new(); - public int PendingCount => _pending.Count; + public int PendingCount => _pendingQueue.Count; public int ActiveCount => _active.Count; @@ -208,46 +199,46 @@ public void RequestCancellationIfActive(DocumentId documentId) public void Enqueue(DocumentId documentId) { - if (_hash.Add(documentId)) + if (_pendingHash.Add(documentId)) { - _pending.Enqueue(documentId); + _pendingQueue.Enqueue(documentId); - if (_pending.Count > MaximumPendingCount) - MaximumPendingCount = _pending.Count; + if (_pendingQueue.Count > MaximumPendingCount) + MaximumPendingCount = _pendingQueue.Count; } } public bool IsEnqueued(DocumentId documentId) => - _hash.Contains(documentId); + _pendingHash.Contains(documentId); public bool IsActive(DocumentId documentId) => _active.ContainsKey(documentId); public void Remove(DocumentId documentId) { - if (_hash.Contains(documentId)) + if (_pendingHash.Contains(documentId)) { - _hash.Remove(documentId); + _pendingHash.Remove(documentId); - var backgroundQueueItems = _pending.ToList(); + var backgroundQueueItems = _pendingQueue.ToList(); - _pending.Clear(); + _pendingQueue.Clear(); foreach (var item in backgroundQueueItems) { if (item != documentId) - _pending.Enqueue(item); + _pendingQueue.Enqueue(item); } } } public bool TryDequeue([NotNullWhen(true)] out DocumentId? documentId, [NotNullWhen(true)] out CancellationTokenSource? cancellationTokenSource) { - if (_pending.Count > 0) + if (_pendingQueue.Count > 0) { - documentId = _pending.Dequeue(); + documentId = _pendingQueue.Dequeue(); - _hash.Remove(documentId); + _pendingHash.Remove(documentId); if (!_active.TryGetValue(documentId, out var cancellationTokenSources)) _active[documentId] = cancellationTokenSources = new List(); @@ -285,7 +276,7 @@ public void WorkComplete(DocumentId documentId, CancellationToken cancellationTo _active.Remove(documentId); var isReenqueued = cancellationToken.IsCancellationRequested - && (_hash.Contains(documentId) || _active.ContainsKey(documentId)); + && (_pendingHash.Contains(documentId) || _active.ContainsKey(documentId)); if (!isReenqueued) { @@ -304,10 +295,10 @@ public void WorkComplete(DocumentId documentId, CancellationToken cancellationTo public Task GetWaiter() { - if (_active.Count == 0 && _pending.Count == 0) + if (_active.Count == 0 && _pendingQueue.Count == 0) return Task.CompletedTask; - var documentIds = new HashSet(_hash.Concat(_active.Keys)); + var documentIds = new HashSet(_pendingHash.Concat(_active.Keys)); var waiter = _waiters.FirstOrDefault(x => x.DocumentIds.SetEquals(documentIds)); diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/TestEventEmitter.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/TestEventEmitter.cs index ba5fa9d46c..fddcb1dce4 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/TestEventEmitter.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/TestEventEmitter.cs @@ -4,6 +4,7 @@ using System.Linq.Expressions; using System.Threading; using System.Threading.Tasks; +using Newtonsoft.Json; using OmniSharp.Eventing; namespace OmniSharp.Roslyn.CSharp.Tests @@ -37,7 +38,9 @@ public async Task ExpectForEmitted(Expression> predicate) } catch (OperationCanceledException) { - throw new InvalidOperationException($"Timeout reached before expected event count reached before prediction {predicate} came true, current diagnostics '{String.Join(";", _messages)}'"); + var messages = string.Join(";", _messages.Select(x => JsonConvert.SerializeObject(x))); + + throw new InvalidOperationException($"Timeout reached before expected event count reached before prediction {predicate} came true, current diagnostics '{messages}'"); } finally { From ddbab60ae0cb468f3392a87d4a6d352568862edb Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Thu, 12 May 2022 11:32:53 +1200 Subject: [PATCH 13/15] Fix deadlock --- .../Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs index 457065ee8d..ce83a4f5d1 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -304,7 +304,7 @@ public Task GetWaiter() if (waiter == default) { - waiter = (documentIds, new TaskCompletionSource()); + waiter = (documentIds, new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously)); _waiters.Add(waiter); } From a180b16a955a7ee6e5f3f6a0695f150300948689 Mon Sep 17 00:00:00 2001 From: Philip Cox Date: Thu, 12 May 2022 11:44:32 +1200 Subject: [PATCH 14/15] Fix merge --- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 48 ------------------- 1 file changed, 48 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 5c0306804f..c83db70ff7 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -32,11 +32,7 @@ public class CSharpDiagnosticWorkerWithAnalyzers : ICsDiagnosticWorker, IDisposa private readonly OmniSharpOptions _options; private readonly OmniSharpWorkspace _workspace; -<<<<<<< HEAD private int _projectCount = 0; -======= - private const int WorkerWait = 250; ->>>>>>> master public CSharpDiagnosticWorkerWithAnalyzers( OmniSharpWorkspace workspace, @@ -122,39 +118,7 @@ private async Task Worker() item = await _workQueue.TakeWorkAsync(); (documentId, cancellationToken, workType, documentCount, remaining) = item; -<<<<<<< HEAD if (workType == AnalyzerWorkType.Background) -======= - var documents = _workQueue - .TakeWork(workType) - .Select(documentId => (projectId: solution.GetDocument(documentId)?.Project?.Id, documentId)) - .Where(x => x.projectId != null) - .ToImmutableArray(); - - if (documents.IsEmpty) - { - _workQueue.WorkComplete(workType); - - await Task.Delay(WorkerWait); - - continue; - } - - var documentCount = documents.Length; - var documentCountRemaining = documentCount; - - // event every percentage increase, or every 10th if there are fewer than 1000 - var eventEvery = Math.Max(10, documentCount / 100); - - var documentsGroupedByProjects = documents - .GroupBy(x => x.projectId, x => x.documentId) - .ToImmutableArray(); - var projectCount = documentsGroupedByProjects.Length; - - EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Started, projectCount, documentCount, documentCountRemaining); - - void decrementDocumentCountRemaining() ->>>>>>> master { // event every percentage increase, or every 10th if there are fewer than 1000 var eventEvery = Math.Max(10, documentCount / 100); @@ -180,17 +144,10 @@ void decrementDocumentCountRemaining() if (remaining == 0) EventIfBackgroundWork(workType, BackgroundDiagnosticStatus.Finished, _projectCount, documentCount, remaining); } -<<<<<<< HEAD } catch (OperationCanceledException) when (cancellationToken != null && cancellationToken.Value.IsCancellationRequested) { _logger.LogInformation($"Analyzer work cancelled."); -======= - - _workQueue.WorkComplete(workType); - - await Task.Delay(WorkerWait); ->>>>>>> master } catch (Exception ex) { @@ -265,12 +222,7 @@ public async Task> AnalyzeDocumentAsync(Document documen var allAnalyzers = GetAnalyzersForProject(project); var compilation = await project.GetCompilationAsync(cancellationToken); -<<<<<<< HEAD return await AnalyzeDocument(project, allAnalyzers, compilation, CreateAnalyzerOptions(document.Project), document, cancellationToken); -======= - cancellationToken.ThrowIfCancellationRequested(); - return await AnalyzeDocument(project, allAnalyzers, compilation, CreateAnalyzerOptions(document.Project), document); ->>>>>>> master } public async Task> AnalyzeProjectsAsync(Project project, CancellationToken cancellationToken) From 3c3bc9387bc655c6928cf7db8822b0d0ae7180d4 Mon Sep 17 00:00:00 2001 From: Joey Robichaud Date: Mon, 25 Sep 2023 10:22:59 -0700 Subject: [PATCH 15/15] Use combined cancellation token throughout AnalyzeDocument. - Apply suggestions. --- .../Diagnostics/AsyncAnalyzerWorkQueue.cs | 26 ++++++++++--------- .../CSharpDiagnosticWorkerWithAnalyzers.cs | 8 +++--- .../AsyncAnalyzerWorkerQueueFacts.cs | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs index ce83a4f5d1..3ef53aec9b 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs @@ -13,7 +13,7 @@ namespace OmniSharp.Roslyn.CSharp.Workers.Diagnostics public class AsyncAnalyzerWorkQueue { private readonly object _lock = new(); - private readonly Queue _forground = new(); + private readonly Queue _foreground = new(); private readonly Queue _background = new(); private readonly ILogger _logger; private TaskCompletionSource _takeWorkWaiter = new(TaskCreationOptions.RunContinuationsAsynchronously); @@ -28,7 +28,7 @@ public int PendingCount get { lock (_lock) - return _forground.PendingCount + _background.PendingCount; + return _foreground.PendingCount + _background.PendingCount; } } @@ -38,15 +38,16 @@ public void PutWork(IReadOnlyCollection documentIds, AnalyzerWorkTyp { foreach (var documentId in documentIds) { - _forground.RequestCancellationIfActive(documentId); + _foreground.RequestCancellationIfActive(documentId); _background.RequestCancellationIfActive(documentId); if (workType == AnalyzerWorkType.Foreground) - _forground.Enqueue(documentId); + _foreground.Enqueue(documentId); else if (workType == AnalyzerWorkType.Background) _background.Enqueue(documentId); } + // Complete the work waiter task to allow work to be taken from the queue. if (!_takeWorkWaiter.Task.IsCompleted) _takeWorkWaiter.SetResult(null); } @@ -62,15 +63,15 @@ public async Task TakeWorkAsync(CancellationToken cancellationToken = lock (_lock) { - if (_forground.TryDequeue(out var documentId, out var cancellationTokenSource)) + if (_foreground.TryDequeue(out var documentId, out var cancellationTokenSource)) { return new QueueItem ( DocumentId: documentId, CancellationToken: cancellationTokenSource.Token, AnalyzerWorkType: AnalyzerWorkType.Foreground, - DocumentCount: _forground.MaximumPendingCount, - DocumentCountRemaining: _forground.PendingCount + DocumentCount: _foreground.MaximumPendingCount, + DocumentCountRemaining: _foreground.PendingCount ); } else if (_background.TryDequeue(out documentId, out cancellationTokenSource)) @@ -85,12 +86,15 @@ public async Task TakeWorkAsync(CancellationToken cancellationToken = ); } - if (_forground.PendingCount == 0 && _background.PendingCount == 0 && _takeWorkWaiter.Task.IsCompleted) + if (_foreground.PendingCount == 0 && _background.PendingCount == 0 && _takeWorkWaiter.Task.IsCompleted) _takeWorkWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); awaitTask = _takeWorkWaiter.Task; } + // There is no chance of the default cancellation token being cancelled, so we can + // simply wait for work to be queued. Otherwise, we need to handle the case that the + // token is cancelled before we have work to return. if (cancellationToken == default) { await awaitTask.ConfigureAwait(false); @@ -103,9 +107,7 @@ public async Task TakeWorkAsync(CancellationToken cancellationToken = { await Task.WhenAny(awaitTask, tcs.Task).ConfigureAwait(false); } - } - } } @@ -114,7 +116,7 @@ public void WorkComplete(QueueItem item) lock (_lock) { if (item.AnalyzerWorkType == AnalyzerWorkType.Foreground) - _forground.WorkComplete(item.DocumentId, item.CancellationToken); + _foreground.WorkComplete(item.DocumentId, item.CancellationToken); else if (item.AnalyzerWorkType == AnalyzerWorkType.Background) _background.WorkComplete(item.DocumentId, item.CancellationToken); } @@ -128,7 +130,7 @@ public async Task WaitForegroundWorkComplete(CancellationToken cancellationToken Task waitForgroundTask; lock (_lock) - waitForgroundTask = _forground.GetWaiter(); + waitForgroundTask = _foreground.GetWaiter(); if (waitForgroundTask.IsCompleted) return; diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 03d915eadd..ddf73f6344 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -316,12 +316,12 @@ private async Task> AnalyzeDocument(Project project, reportSuppressedDiagnostics: false)); Task> semanticDiagnosticsWithAnalyzers = compilationWithAnalyzers - .GetAnalyzerSemanticDiagnosticsAsync(documentSemanticModel, filterSpan: null, cancellationToken); + .GetAnalyzerSemanticDiagnosticsAsync(documentSemanticModel, filterSpan: null, combinedCancellation.Token); Task> syntaxDiagnosticsWithAnalyzers = compilationWithAnalyzers - .GetAnalyzerSyntaxDiagnosticsAsync(syntaxTree, cancellationToken); + .GetAnalyzerSyntaxDiagnosticsAsync(syntaxTree, combinedCancellation.Token); - ImmutableArray documentSemanticDiagnostics = documentSemanticModel.GetDiagnostics(null, cancellationToken); + ImmutableArray documentSemanticDiagnostics = documentSemanticModel.GetDiagnostics(null, combinedCancellation.Token); await Task.WhenAll(syntaxDiagnosticsWithAnalyzers, semanticDiagnosticsWithAnalyzers); @@ -331,7 +331,7 @@ private async Task> AnalyzeDocument(Project project, .Concat(documentSemanticDiagnostics) .ToImmutableArray(); } - catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + catch (OperationCanceledException) when (combinedCancellation.Token.IsCancellationRequested) { throw; } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs index 53acbc2734..7fad2a433f 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs @@ -136,7 +136,7 @@ public async Task WhenMultipleThreadsAreConsumingAnalyzerWorkerQueueItWorksAsExp } [Fact] - public async Task WheNewnWorkIsAddedAgainWhenPreviousIsAnalysing_ThenDontWaitAnotherOneToGetReady() + public async Task WhenNewWorkIsAddedAgainWhenPreviousIsAnalysing_ThenDontWaitAnotherOneToGetReady() { var queue = new AsyncAnalyzerWorkQueue(new LoggerFactory()); var document1 = CreateTestDocumentId();