From c6fcbdfa84dd66e9b507633aac277ef0601b3509 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Thu, 21 Oct 2021 14:01:19 -0700 Subject: [PATCH 1/4] Stop crashing VS when logging errors. (#5646) * Stop crashing VS when logging errors. - Ultimately this is more of a workaround for this issue: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1405849. But let me share some insight as to how we got here. When I initially ["protected VS from crashing"](https://github.com/dotnet/razor-tooling/commit/95b33df1d60a192f1ac6e3f8417f9fa416578f6d) due to the compiler bug I added logic that would `LogError` in the case that unexpected exceptions would occur. Now the problem with my approach is that our Language Server logger can potentially explode if the language server is in the midst of shutting down. Therefore, what would happen is the compiler would unexpectedly throw, we'd then try to log an error and if we were in a shutting down state that logging of an error would then throw again resulting in VS crashing. - Went from logging an error to an unobserved task exception. This means things will still throw; however, they'll be translated into unobserved task exceptions. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1420246 * Address code review comments. --- .../RazorDiagnosticsPublisher.cs | 97 +++++++++++-------- 1 file changed, 55 insertions(+), 42 deletions(-) diff --git a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDiagnosticsPublisher.cs b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDiagnosticsPublisher.cs index 2a09bb735e..eeb79bf86e 100644 --- a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDiagnosticsPublisher.cs +++ b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDiagnosticsPublisher.cs @@ -110,56 +110,62 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer } } -#pragma warning disable VSTHRD100 // Avoid async void methods - private async void DocumentClosedTimer_Tick(object state) -#pragma warning restore VSTHRD100 // Avoid async void methods + private void DocumentClosedTimer_Tick(object state) { - try - { - await _projectSnapshotManagerDispatcher.RunOnDispatcherThreadAsync( - ClearClosedDocuments, - CancellationToken.None).ConfigureAwait(false); - } - catch (Exception ex) - { - // Swallow exception to not crash VS (this is an async void method) - _logger.LogError(ex, "Background diagnostic publisher for Razor failed to publish diagnostics for an unexpected reason."); - } + _ = DocumentClosedTimer_TickAsync(CancellationToken.None); + } + + private async Task DocumentClosedTimer_TickAsync(CancellationToken cancellationToken) + { + await _projectSnapshotManagerDispatcher.RunOnDispatcherThreadAsync( + ClearClosedDocuments, + cancellationToken).ConfigureAwait(false); } // Internal for testing internal void ClearClosedDocuments() { - lock (PublishedDiagnostics) + try { - var publishedDiagnostics = new Dictionary>(PublishedDiagnostics); - foreach (var entry in publishedDiagnostics) + lock (PublishedDiagnostics) { - if (!_projectManager.IsDocumentOpen(entry.Key)) + var publishedDiagnostics = new Dictionary>(PublishedDiagnostics); + foreach (var entry in publishedDiagnostics) { - // Document is now closed, we shouldn't track its diagnostics anymore. - PublishedDiagnostics.Remove(entry.Key); - - // If the last published diagnostics for the document were > 0 then we need to clear them out so the user - // doesn't have a ton of closed document errors that they can't get rid of. - if (entry.Value.Count > 0) + if (!_projectManager.IsDocumentOpen(entry.Key)) { - PublishDiagnosticsForFilePath(entry.Key, Array.Empty()); + // Document is now closed, we shouldn't track its diagnostics anymore. + PublishedDiagnostics.Remove(entry.Key); + + // If the last published diagnostics for the document were > 0 then we need to clear them out so the user + // doesn't have a ton of closed document errors that they can't get rid of. + if (entry.Value.Count > 0) + { + PublishDiagnosticsForFilePath(entry.Key, Array.Empty()); + } + } + } + + _documentClosedTimer?.Dispose(); + _documentClosedTimer = null; + + if (PublishedDiagnostics.Count > 0) + { + lock (_work) + { + // There's no way for us to know when a document is closed at this layer. Therefore, we need to poll every X seconds + // and check if the currently tracked documents are closed. In practice this work is super minimal. + StartDocumentClosedCheckTimer(); } } } - - _documentClosedTimer?.Dispose(); - _documentClosedTimer = null; - - if (PublishedDiagnostics.Count > 0) + } + finally + { + lock (PublishedDiagnostics) { - lock (_work) - { - // There's no way for us to know when a document is closed at this layer. Therefore, we need to poll every X seconds - // and check if the currently tracked documents are closed. In practice this work is super minimal. - StartDocumentClosedCheckTimer(); - } + _documentClosedTimer?.Dispose(); + _documentClosedTimer = null; } } } @@ -198,9 +204,12 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer } } -#pragma warning disable VSTHRD100 // Avoid async void methods - private async void WorkTimer_Tick(object state) -#pragma warning restore VSTHRD100 // Avoid async void methods + private void WorkTimer_Tick(object state) + { + _ = WorkTimer_TickAsync(CancellationToken.None); + } + + private async Task WorkTimer_TickAsync(CancellationToken cancellationToken) { try { @@ -232,10 +241,14 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer } } } - catch (Exception ex) + finally { - // Swallow exception to not crash VS (this is an async void method) - _logger.LogError(ex, "Background diagnostic publisher for Razor failed to publish diagnostics for an unexpected reason."); + lock (_work) + { + // Resetting the timer allows another batch of work to start. + _workTimer.Dispose(); + _workTimer = null; + } } } From d540566de0b1eb5d3a274fd40824ef03fe8650fe Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Thu, 21 Oct 2021 15:33:57 -0700 Subject: [PATCH 2/4] Fix failing diagnostic publisher tests. - Only clear timers on exceptions. --- .../RazorDiagnosticsPublisher.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDiagnosticsPublisher.cs b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDiagnosticsPublisher.cs index eeb79bf86e..484df6a855 100644 --- a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDiagnosticsPublisher.cs +++ b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDiagnosticsPublisher.cs @@ -160,13 +160,15 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer } } } - finally + catch { lock (PublishedDiagnostics) { _documentClosedTimer?.Dispose(); _documentClosedTimer = null; } + + throw; } } @@ -241,7 +243,7 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer } } } - finally + catch { lock (_work) { @@ -249,6 +251,8 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer _workTimer.Dispose(); _workTimer = null; } + + throw; } } From de9577ea0597ff87feda35ae2ee7cd35dc49f3b2 Mon Sep 17 00:00:00 2001 From: Allison Chou Date: Thu, 7 Oct 2021 14:43:35 -0700 Subject: [PATCH 3/4] Update azure-pipelines.yml --- azure-pipelines.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index c376535197..7ebaa2711c 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -48,7 +48,7 @@ stages: parameters: LclSource: lclFilesfromPackage LclPackageId: 'LCL-JUNO-PROD-RAZORTOOL' - MirrorRepo: aspnetcore-tooling + MirrorRepo: razor-tooling - template: /eng/common/templates/jobs/jobs.yml parameters: @@ -96,7 +96,7 @@ stages: enablePublishBuildArtifacts: true enablePublishTestResults: true enableTelemetry: true - helixRepo: dotnet/aspnetcore-tooling + helixRepo: dotnet/razor-tooling helixType: build.product/ # enableMicrobuild can't be read from a user-defined variable (Azure DevOps limitation) ${{ if and(ne(variables['System.TeamProject'], 'public'), notin(variables['Build.Reason'], 'PullRequest')) }}: @@ -149,7 +149,7 @@ stages: /p:OfficialBuildId=$(Build.BuildNumber) /p:ManifestBuildBranch=$(Build.SourceBranchName) /p:ManifestBuildNumber=$(Build.BuildNumber) - /p:VisualStudioDropName=Products/dotnet/aspnetcore-tooling/$(Build.SourceBranchName)/$(Build.BuildNumber) + /p:VisualStudioDropName=Products/dotnet/razor-tooling/$(Build.SourceBranchName)/$(Build.BuildNumber) - ${{ if and(ne(variables['System.TeamProject'], 'public'), notin(variables['Build.Reason'], 'PullRequest')) }}: - group: DotNet-Blob-Feed From 3319c75f898522ff4dc9cf0c70035c284264a1de Mon Sep 17 00:00:00 2001 From: Allison Chou Date: Thu, 7 Oct 2021 14:47:55 -0700 Subject: [PATCH 4/4] Update azure-pipelines-richnav.yml --- azure-pipelines-richnav.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azure-pipelines-richnav.yml b/azure-pipelines-richnav.yml index 1dba18887e..2481b526cb 100644 --- a/azure-pipelines-richnav.yml +++ b/azure-pipelines-richnav.yml @@ -23,7 +23,7 @@ stages: jobs: - template: /eng/common/templates/jobs/jobs.yml parameters: - helixRepo: dotnet/aspnetcore-tooling + helixRepo: dotnet/razor-tooling helixType: build.product/ enableRichCodeNavigation: true richCodeNavigationEnvironment: 'production'