Treat missing .ConfigureAwait(false) as errors (and fix)
#633
Conversation
|
I've had users confirm that this change solved PowerShell/vscode-powershell#3394, PowerShell/vscode-powershell#3410, and PowerShell/vscode-powershell#3415. Note that I first applied this change to the PowerShell Editor Services code base in PowerShell/PowerShellEditorServices#1533, which alone did not solve the issue. The same bug was found and fixed in the past with PowerShell/PowerShellEditorServices#1149 which gave me the idea. |
|
Ah, I need to fix the tests too, I guess those didn't build from the build script. |
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.
andyleejordan
force-pushed
the
andschwa/configure-await
branch
from
af69d4e to
3493200
Compare
Codecov Report
@@ Coverage Diff @@
## master #633 +/- ##
==========================================
- Coverage 69.37% 69.33% -0.04%
==========================================
Files 257 257
Lines 12576 12576
Branches 848 848
==========================================
- Hits 8724 8719 -5
- Misses 3582 3587 +5
Partials 270 270
Continue to review full report at Codecov.
|
|
Hey, it's not mysterious! It's a threading deadlock 😆 |
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 istruewhich roughly means that the awaited tasks will only run on their own context. So when the context is changed (because ofSystem.Windows.Formsor other code introducing a new synchronization context) the task will never be continued, resulting in this hang. By configuring this tofalsewe 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.