From 349320056bc6f03f364a4242231eec6cdf16dfda Mon Sep 17 00:00:00 2001 From: Andrew Schwartzmeyer Date: Tue, 10 Aug 2021 09:46:34 -0700 Subject: [PATCH] Treat missing `.ConfigureAwait(false)` as errors (and fix) I enabled the new .NET Analyzer for the project and then used an Editor Config rule to treat CA2007 as an error. This diagnostic is for any awaited task that does not have an explicit trailing `.ConfigureAwait(false)` call, an annoying but very necessary configuration for asynchronous .NET library code (such as OmniSharp). The bugs caused by these missing calls are strange. In the case of PowerShell Editor Services, an LSP server using OmniSharp and powering the PowerShell Extension for VS Code, it showed up as a hang when the server executed user code that used objects from `System.Windows.Forms`. This is because that .NET library sets its own synchronization context, which is exactly where `ConfigureAwait(continueOnCapturedContext: false)` comes into play. The default value is `true` which roughly means that the awaited tasks will only run on their own context. So when the context is changed (because of `System.Windows.Forms` or other code introducing a new synchronization context) the task will never be continued, resulting in this hang. By configuring this to `false` we allow the tasks to continue regardless of the new context. See this .NET blog post for more details: https://devblogs.microsoft.com/dotnet/configureawait-faq/ Note that elsewhere in the codebase we've been very careful to set it to false, but we've not been perfect. Treating this diagnostic as an error will allow us to be as perfect as possible about it. --- .editorconfig | 4 ++++ Directory.Build.props | 2 ++ sample/SampleServer/Program.cs | 16 ++++++++-------- sample/SampleServer/SemanticTokensHandler.cs | 8 ++++---- sample/SampleServer/TextDocumentHandler.cs | 14 +++++++------- src/Dap.Client/DebugAdapterClient.cs | 2 +- src/Dap.Testing/DebugAdapterProtocolTestBase.cs | 2 +- src/JsonRpc/OutputHandler.cs | 2 +- .../Features/Document/CallHierarchyFeature.cs | 2 +- src/Server/LanguageServer.Shutdown.cs | 4 ++-- test/.editorconfig | 1 + 11 files changed, 32 insertions(+), 25 deletions(-) diff --git a/.editorconfig b/.editorconfig index 4cbf46047..81d812533 100644 --- a/.editorconfig +++ b/.editorconfig @@ -128,7 +128,11 @@ resharper_web_config_module_not_resolved_highlighting=warning resharper_web_config_type_not_resolved_highlighting=warning resharper_web_config_wrong_module_highlighting=warning +# .NET Analzyer settings +# VSTHRD200: Use "Async" suffix for awaitable methods dotnet_diagnostic.VSTHRD200.severity = none +# CA2007: Do not directly await a Task +dotnet_diagnostic.CA2007.severity = error [*.{cs,cshtml}] charset=utf-8 diff --git a/Directory.Build.props b/Directory.Build.props index 5e69a2809..2b19c9ec0 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -13,6 +13,8 @@ https://github.com/OmniSharp/csharp-language-server-protocol lsp;language server;language server protocol;language client;language server client $(MSBuildThisFileDirectory)\lsp.snk + + true true diff --git a/sample/SampleServer/Program.cs b/sample/SampleServer/Program.cs index fa2fabf71..d86b96a49 100644 --- a/sample/SampleServer/Program.cs +++ b/sample/SampleServer/Program.cs @@ -83,7 +83,7 @@ private static async Task MainAsync(string[] args) ); workDone = manager; - await Task.Delay(2000); + await Task.Delay(2000).ConfigureAwait(false); manager.OnNext( new WorkDoneProgressReport { @@ -102,7 +102,7 @@ private static async Task MainAsync(string[] args) } ); - await Task.Delay(2000); + await Task.Delay(2000).ConfigureAwait(false); workDone.OnNext( new WorkDoneProgressReport { @@ -115,12 +115,12 @@ private static async Task MainAsync(string[] args) ) .OnStarted( async (languageServer, token) => { - using var manager = await languageServer.WorkDoneManager.Create(new WorkDoneProgressBegin { Title = "Doing some work..." }); + using var manager = await languageServer.WorkDoneManager.Create(new WorkDoneProgressBegin { Title = "Doing some work..." }).ConfigureAwait(false); manager.OnNext(new WorkDoneProgressReport { Message = "doing things..." }); - await Task.Delay(10000); + await Task.Delay(10000).ConfigureAwait(false); manager.OnNext(new WorkDoneProgressReport { Message = "doing things... 1234" }); - await Task.Delay(10000); + await Task.Delay(10000).ConfigureAwait(false); manager.OnNext(new WorkDoneProgressReport { Message = "doing things... 56789" }); var logger = languageServer.Services.GetService>(); @@ -130,7 +130,7 @@ private static async Task MainAsync(string[] args) }, new ConfigurationItem { Section = "terminal", } - ); + ).ConfigureAwait(false); var baseConfig = new JObject(); foreach (var config in languageServer.Configuration.AsEnumerable()) @@ -149,9 +149,9 @@ private static async Task MainAsync(string[] args) logger.LogInformation("Scoped Config: {Config}", scopedConfig); } ) - ); + ).ConfigureAwait(false); - await server.WaitForExit; + await server.WaitForExit.ConfigureAwait(false); } } diff --git a/sample/SampleServer/SemanticTokensHandler.cs b/sample/SampleServer/SemanticTokensHandler.cs index 30b0b0c41..6f1a5db36 100644 --- a/sample/SampleServer/SemanticTokensHandler.cs +++ b/sample/SampleServer/SemanticTokensHandler.cs @@ -25,7 +25,7 @@ public SemanticTokensHandler(ILogger logger) => SemanticTokensParams request, CancellationToken cancellationToken ) { - var result = await base.Handle(request, cancellationToken); + var result = await base.Handle(request, cancellationToken).ConfigureAwait(false); return result; } @@ -33,7 +33,7 @@ public SemanticTokensHandler(ILogger logger) => SemanticTokensRangeParams request, CancellationToken cancellationToken ) { - var result = await base.Handle(request, cancellationToken); + var result = await base.Handle(request, cancellationToken).ConfigureAwait(false); return result; } @@ -42,7 +42,7 @@ public SemanticTokensHandler(ILogger logger) => CancellationToken cancellationToken ) { - var result = await base.Handle(request, cancellationToken); + var result = await base.Handle(request, cancellationToken).ConfigureAwait(false); return result; } @@ -54,7 +54,7 @@ CancellationToken cancellationToken using var typesEnumerator = RotateEnum(SemanticTokenType.Defaults).GetEnumerator(); using var modifiersEnumerator = RotateEnum(SemanticTokenModifier.Defaults).GetEnumerator(); // you would normally get this from a common source that is managed by current open editor, current active editor, etc. - var content = await File.ReadAllTextAsync(DocumentUri.GetFileSystemPath(identifier), cancellationToken); + var content = await File.ReadAllTextAsync(DocumentUri.GetFileSystemPath(identifier), cancellationToken).ConfigureAwait(false); await Task.Yield(); foreach (var (line, text) in content.Split('\n').Select((text, line) => (line, text))) diff --git a/sample/SampleServer/TextDocumentHandler.cs b/sample/SampleServer/TextDocumentHandler.cs index 3d78c6f9b..701b67278 100644 --- a/sample/SampleServer/TextDocumentHandler.cs +++ b/sample/SampleServer/TextDocumentHandler.cs @@ -51,7 +51,7 @@ public override async Task Handle(DidOpenTextDocumentParams notification, { await Task.Yield(); _logger.LogInformation("Hello world!"); - await _configuration.GetScopedConfiguration(notification.TextDocument.Uri, token); + await _configuration.GetScopedConfiguration(notification.TextDocument.Uri, token).ConfigureAwait(false); return Unit.Value; } @@ -84,7 +84,7 @@ CancellationToken cancellationToken ) { // you would normally get this from a common source that is managed by current open editor, current active editor, etc. - var content = await File.ReadAllTextAsync(DocumentUri.GetFileSystemPath(request), cancellationToken); + var content = await File.ReadAllTextAsync(DocumentUri.GetFileSystemPath(request), cancellationToken).ConfigureAwait(false); var lines = content.Split('\n'); var symbols = new List(); for (var lineIndex = 0; lineIndex < lines.Length; lineIndex++) @@ -160,7 +160,7 @@ CancellationToken cancellationToken using var partialResults = _progressManager.For(request, cancellationToken); if (partialResults != null) { - await Task.Delay(2000, cancellationToken); + await Task.Delay(2000, cancellationToken).ConfigureAwait(false); reporter.OnNext( new WorkDoneProgressReport { @@ -168,7 +168,7 @@ CancellationToken cancellationToken Percentage = 20 } ); - await Task.Delay(500, cancellationToken); + await Task.Delay(500, cancellationToken).ConfigureAwait(false); reporter.OnNext( new WorkDoneProgressReport { @@ -176,7 +176,7 @@ CancellationToken cancellationToken Percentage = 40 } ); - await Task.Delay(500, cancellationToken); + await Task.Delay(500, cancellationToken).ConfigureAwait(false); reporter.OnNext( new WorkDoneProgressReport { @@ -184,7 +184,7 @@ CancellationToken cancellationToken Percentage = 50 } ); - await Task.Delay(500, cancellationToken); + await Task.Delay(500, cancellationToken).ConfigureAwait(false); partialResults.OnNext( new[] { @@ -209,7 +209,7 @@ CancellationToken cancellationToken Percentage = 70 } ); - await Task.Delay(500, cancellationToken); + await Task.Delay(500, cancellationToken).ConfigureAwait(false); reporter.OnNext( new WorkDoneProgressReport { diff --git a/src/Dap.Client/DebugAdapterClient.cs b/src/Dap.Client/DebugAdapterClient.cs index f2ec06604..3141c0605 100644 --- a/src/Dap.Client/DebugAdapterClient.cs +++ b/src/Dap.Client/DebugAdapterClient.cs @@ -154,7 +154,7 @@ await DebugAdapterEventingHelper.Run( token ).ConfigureAwait(false); - await _initializedComplete.ToTask(token, _scheduler); + await _initializedComplete.ToTask(token, _scheduler).ConfigureAwait(false); await DebugAdapterEventingHelper.Run( _startedDelegates, diff --git a/src/Dap.Testing/DebugAdapterProtocolTestBase.cs b/src/Dap.Testing/DebugAdapterProtocolTestBase.cs index 2cbe87d4e..14d12b7f6 100644 --- a/src/Dap.Testing/DebugAdapterProtocolTestBase.cs +++ b/src/Dap.Testing/DebugAdapterProtocolTestBase.cs @@ -79,7 +79,7 @@ Action serverOptionsAction return await Observable.FromAsync(_client.Initialize) .ForkJoin(Observable.FromAsync(_server.Initialize), (_, _) => ( _client, _server )) - .ToTask(CancellationToken); + .ToTask(CancellationToken).ConfigureAwait(false); } } } diff --git a/src/JsonRpc/OutputHandler.cs b/src/JsonRpc/OutputHandler.cs index f20d06d3b..4402c7c28 100644 --- a/src/JsonRpc/OutputHandler.cs +++ b/src/JsonRpc/OutputHandler.cs @@ -132,7 +132,7 @@ private async Task ProcessOutputStream(CancellationToken cancellationToken) { do { - var value = await _queue.ReadAsync(cancellationToken); + var value = await _queue.ReadAsync(cancellationToken).ConfigureAwait(false); if (value is ITraceData traceData) { _activityTracingStrategy?.ApplyOutgoing(traceData); diff --git a/src/Protocol/Features/Document/CallHierarchyFeature.cs b/src/Protocol/Features/Document/CallHierarchyFeature.cs index b0d9b5aef..cdfa8096c 100644 --- a/src/Protocol/Features/Document/CallHierarchyFeature.cs +++ b/src/Protocol/Features/Document/CallHierarchyFeature.cs @@ -393,7 +393,7 @@ protected CallHierarchyHandlerBase() : this(Guid.NewGuid()) public sealed override async Task?> Handle(CallHierarchyPrepareParams request, CancellationToken cancellationToken) { - var response = await HandlePrepare(request, cancellationToken); + var response = await HandlePrepare(request, cancellationToken).ConfigureAwait(false); return Container.From(response?.Select(CallHierarchyItem.From)!); } diff --git a/src/Server/LanguageServer.Shutdown.cs b/src/Server/LanguageServer.Shutdown.cs index 9f260c60d..7b52b0ae8 100644 --- a/src/Server/LanguageServer.Shutdown.cs +++ b/src/Server/LanguageServer.Shutdown.cs @@ -24,8 +24,8 @@ public partial class LanguageServer : IExitHandler, IShutdownHandler #pragma warning disable VSTHRD100 public async void ForcefulShutdown() { - await ( (IShutdownHandler) this ).Handle(ShutdownParams.Instance, CancellationToken.None); - await ( (IExitHandler) this ).Handle(ExitParams.Instance, CancellationToken.None); + await ( (IShutdownHandler) this ).Handle(ShutdownParams.Instance, CancellationToken.None).ConfigureAwait(false); + await ( (IExitHandler) this ).Handle(ExitParams.Instance, CancellationToken.None).ConfigureAwait(false); } async Task IRequestHandler.Handle(ExitParams request, CancellationToken token) diff --git a/test/.editorconfig b/test/.editorconfig index e076c5fc2..f508dbdfa 100644 --- a/test/.editorconfig +++ b/test/.editorconfig @@ -1,4 +1,5 @@ [*] +dotnet_diagnostic.CA2007.severity = none dotnet_diagnostic.CS0618.severity = none dotnet_diagnostic.CS4014.severity = none dotnet_diagnostic.CS1998.severity = none