Migrate to Microsoft.VisualStudio.Threading #28781

sharwell · 2018-07-23T16:57:40Z

This change implements the first step towards Roslyn working in applications using vs-threading with reduced risk of concurrency bugs. Recommend reviewing by commit after looking at the full list to get an idea for the overall direction.

Most of the work in this change involves internal propagation of the IThreadingContext instance, which provides access to the JoinableTaskFactory used in Roslyn code.

Following this pull request, several diagnostics remain disabled.

VSTHRD002 (Avoid problematic synchronous waits): Many of the violations reported by this can be fixed by a code fix, but the code fix is a bit buggy and I'm waiting for Preserve assertion when fixing VSTHRD103 microsoft/vs-threading#338 so it's easier to prepare the fixes.
VSTHRD103 (Call async methods when in an async method): This will be sent as a follow-up pull request. The analysis will be improved by Treat 'Synchronously' as a suffix for synchronous methods microsoft/vs-threading#331, but I didn't see any reason why it would block our use of the analyzer.
VSTHRD110 (Observe result of async calls): We need to establish the "Roslyn desired practice for FileAndForget" before enabling this diagnostic.
VSTHRD200 (Use "Async" suffix for async methods): Will send a follow-up PR to enable this and fix all cases that aren't exposed (public API and/or IVT). It affected too many files to include here.
VSTHRD010 (Invoke single-threaded types on Main thread): This diagnostic is enabled, but its accuracy relies heavily on configuration files provided separately. The configuration for VSSDK types ships with the VSSDK analyzers, which are not yet enabled for the full Roslyn build.

Known follow-up issues (GitHub issues will be linked here once they are available):

Enable VSTHRD002 (see above)
Enable VSTHRD103 (see above)
Enable VSTHRD110 (see above)
Enable VSTHRD200 (see above)
Configure WaitAndGetResult and WaitAndGetResult_CanCallOnBackground as problematic synchronous waits (blocked on Allow extensions for VSTHRD002 (Avoid problematic synchronous waits) microsoft/vs-threading#344)
Review code for additional methods to consider legacy thread switching APIs (blocked on Allow extensions for VSTHRD001 (Avoid legacy thread switching APIs) microsoft/vs-threading#332)
Update VSSDK analyzers to enable VSTHRD010 (see above)
Eliminate ForegroundThreadDataKind, and gut or eliminate ForegroundThreadAffinitizedObject Migrate to Microsoft.VisualStudio.Threading #28781 (comment)
Consider making JoinableTaskContext a required import for ThreadingContext Migrate to Microsoft.VisualStudio.Threading #28781 (comment)
Eliminate JoinableTaskFactoryTaskScheduler, or at least add it to the set of legacy thread switching types Migrate to Microsoft.VisualStudio.Threading #28781 (comment)
PERF: Improve CPSProject_IWorkspaceProjectContext.cs Migrate to Microsoft.VisualStudio.Threading #28781 (comment)
Design, implement, and uniformly use a main thread check/assert method Migrate to Microsoft.VisualStudio.Threading #28781 (comment)
PERF: Remove AbstractPackage.ForegroundObject, and its costly initialization Migrate to Microsoft.VisualStudio.Threading #28781 (comment)
Test devenv /build Migrate to Microsoft.VisualStudio.Threading #28781 (comment)
Audit uses of ContinueWith, SafeContinueWith, StartNew, SafeStartNew, and Task.Run to identify cases where simpler approaches are viable Migrate to Microsoft.VisualStudio.Threading #28781 (comment)
Inconsistent use of TestWorkspace.GetService<T> and TestWorkspace.ExportProvider.GetExportedValue<T> Migrate to Microsoft.VisualStudio.Threading #28781 (comment)

Customer scenario

This is an internal code change that allows dotnet/roslyn to be more easily incorporated into applications that involve user interfaces and leverage the Microsoft.VisualStudio.Threading library for deadlock and reentrancy mitigation and overall best practices.

Bugs this fixes

N/A

Workarounds, if any

Fixing bugs as they occur.

Risk

This change has moderate risk, but I believe it's relatively low risk considering its scope since existing behaviors were preserved where possible. The primary points where I see risk are the following:

Not all portions of code were updated to follow the Three threading rules, and incremental adoption of vs-threading runs a risk of deadlocks when code does not follow these rules.
While the documented precondition of WaitAndGetResult was weakened, the debug assertion it contained was strengthened substantially. This code does not affect release builds, but despite the work in this pull request it may continue to affect debug pull request builds.

Performance impact

Performance should not change substantially as a direct result of this pull request. Over time, performance should improve by allowing more code to use asynchronous operations and more efficient task continuations.

Is this a regression from a previous update?

N/A

Root cause analysis

N/A

How was the bug found?

N/A

Test documentation updated?

N/A

jasonmalinowski · 2018-07-23T17:22:56Z

@sharwell Just to note, #27027 is also moving us to some of these packages as well, with a ruleset file. Just so you're aware.

…ispatcherPriority)

Neme12 · 2018-07-23T22:02:08Z

src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemProvider.cs

@@ -149,7 +149,7 @@ private void StartSearch(INavigateToCallback callback, string searchValue, IImmu
                kinds,
                _cancellationTokenSource.Token);

-            searcher.Search();
+            _ = searcher.SearchAsync();


Doesn't this have an effect of silencing all exceptions?

➡️ It's an intermediate step. VSTHRD110 deals with observing the result of asynchronous operations.

@sharwell intermediate step still needs fixing further?

@jasonmalinowski Covered under the VSTHRD110 follow-up issue.

…dObject

…text.Invalid

src/EditorFeatures/Core.Wpf/Adornments/AdornmentManager.cs

                        {
+                            await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();


src/EditorFeatures/Core.Wpf/IWpfTextViewExtensions.cs

                {
+                    await threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();


src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs

+            allRenameLocationsTask.SafeContinueWithFromAsync(
+                async t =>
+                {
+                    await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(_cancellationTokenSource.Token);


src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs

-                Dispatcher.CurrentDispatcher.BeginInvoke(propagateEditAction, DispatcherPriority.Send, null);
+                ThreadingContext.JoinableTaskFactory.WithPriority(Dispatcher.CurrentDispatcher, DispatcherPriority.Send).RunAsync(async () =>
+                {
+                    await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();


src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs

+                .SafeContinueWithFromAsync(
+                    async t =>
+                    {
+                        await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(_conflictResolutionTaskCancellationSource.Token);


...rFeatures/Core/Implementation/RenameTracking/RenameTrackingTaggerProvider.TrackingSession.cs

                   {
-                       AssertIsForeground();
+                       await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(_cancellationToken);


src/VisualStudio/CSharp/Impl/EventHookup/EventHookupSessionManager_EventHookupSession.cs

                        {
-                            AssertIsForeground();
+                            await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(_cancellationTokenSource.Token);


...sualStudio/Core/Def/Implementation/DesignerAttribute/DesignerAttributeIncrementalAnalyzer.cs

@@ -144,7 +143,13 @@ public void RemoveProject(ProjectId projectId)

            // CPS projects do not support designer attributes.  So we just skip these projects entirely.
            var vsWorkspace = project.Solution.Workspace as VisualStudioWorkspaceImpl;
-            var cps = await Task.Factory.StartNew(() => vsWorkspace?.IsCPSProject(project) == true, cancellationToken, TaskCreationOptions.None, this.ForegroundTaskScheduler).ConfigureAwait(false);
+
+            await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);


src/VisualStudio/Core/Def/Packaging/PackageInstallerServiceFactory.cs

+                    .ContinueWith(
+                        async _ =>
+                        {
+                            await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);


src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs

                        {
+                            await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();


…main thread

CyrusNajmabadi · 2018-08-07T23:06:36Z

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs

                            // since execution is now technically asynchronous
                            // only execute action if project is not disconnected and currently being tracked.
                            if (!_disconnected && this.ProjectTracker.ContainsProject(this))
                            {
                                action();
                            }
                        },
-                        ForegroundTaskScheduler);
+                        CancellationToken.None,
+                        TaskContinuationOptions.ExecuteSynchronously,


different options than usual. intentional?

➡️ Situation is most similar to #28781 (comment), but has a dedicated follow-up item due to #28781 (comment)

CyrusNajmabadi · 2018-08-07T23:09:14Z

My brain really hurts.

jasonmalinowski · 2018-08-08T00:03:49Z

My brain really hurts.

And this is the easy change. 😄

sharwell · 2018-08-08T03:53:48Z

@heejaechang @jaredpar @jasonmalinowski @CyrusNajmabadi @DustinCampbell I replied to the easy items, and will work on making requested changes and finishing my replies tomorrow. Thank you for the detailed reviews. 🏅

* Remove unused test constructors in QuickInfoCommandHandlerAndSourceProvider * Document `IThreadingContext`, `ThreadingContext`, and `DenyExecutionSynchronizationContext` * Ensure `DenyExecutionSynchronizationContext.ThrowIfSwitchOccurred` gets called during test cleanup * Clean up project layering by moving `StubVsEditorAdaptersFactoryService`

jaredpar

The change looks good. I'd probably wait for one more IDE sign off though as I'm a bit out of my depth in a few places.

My biggest complaint is the lack of ConfigureAwait in places. I get that we cant add it because it's custom awaitable structs. But I feel like the readibility goes down a bit. Perhaps once we get a bit more used to the MS.VS.Threading this will go away.

DustinCampbell

Looks pretty good. I'll be interested in the follow ups that come later.

Note that this will almost certainly break insertions due to components that have IVT.

DustinCampbell · 2018-08-09T14:53:31Z

src/EditorFeatures/CSharpTest/Classification/SyntacticTaggerTests.cs

@@ -35,7 +36,7 @@ public async Task TestTagsChangedForEntireFile()
                    workspace.GetService<IForegroundNotificationService>(),
                    AsynchronousOperationListenerProvider.NullListener,
                    null,
-                    new SyntacticClassificationTaggerProvider(null, null, null));
+                    new SyntacticClassificationTaggerProvider(workspace.ExportProvider.GetExportedValue<IThreadingContext>(), null, null, null));


nit: would be nice to name the null arguments.

➡️ #29182

DustinCampbell · 2018-08-09T14:57:28Z

src/EditorFeatures/CSharpTest/Interactive/BraceMatching/InteractiveBraceHighlightingTests.cs

@@ -34,6 +35,7 @@ private IEnumerable<T> Enumerable<T>(params T[] array)
        {
            var view = new Mock<ITextView>();
            var producer = new BraceHighlightingViewTaggerProvider(
+                workspace.ExportProvider.GetExportedValue<IThreadingContext>(),
                workspace.GetService<IBraceMatchingService>(),


TestWorkspace.GetService<TServiceInterface> is just a shortcut to call ExportProvider.GetExportedValue. Should we call one or the other consistently?

➡️ 😦 I certainly should have used workspace.GetService<IThreadingContext>() here for consistency. I'll file a follow-up item related to the confusing pattern(s) we see today with these methods.

DustinCampbell · 2018-08-09T15:07:55Z

src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs

                _cancellationTokenSource.Token,
-                TaskContinuationOptions.OnlyOnRanToCompletion,
-                ForegroundTaskScheduler).CompletesAsyncOperation(asyncToken);
+                TaskContinuationOptions.OnlyOnRanToCompletion | TaskContinuationOptions.ExecuteSynchronously,


It'd be good if this were documented somewhere other than a comment buried in this PR (which has already "unicorned" on me twice! 😄). Since two very smart people have already asked about this pattern here, it'll almost certainly be stumbling block to anyone who happens upon the code. Maybe just add a comment in the code where the pattern is used? E.g. "ExecuteSynchronously is passed to avoid a situation where the continuation to be scheduled on a background thread before immediately switching to the main thread."

DustinCampbell · 2018-08-09T15:10:02Z

src/EditorFeatures/Core.Wpf/InlineRename/InlineRenameSession.cs

                _cancellationTokenSource.Token,
-                TaskContinuationOptions.OnlyOnRanToCompletion,
-                ForegroundTaskScheduler).CompletesAsyncOperation(asyncToken);
+                TaskContinuationOptions.OnlyOnRanToCompletion | TaskContinuationOptions.ExecuteSynchronously,


Or, maybe add a helper method with a good name to handle this situation rather than passing the same flags everytime? The exact same pattern is used below.

DustinCampbell · 2018-08-09T15:13:37Z

src/EditorFeatures/Core.Wpf/QuickInfo/Presentation/QuickInfoPresenter.cs

@@ -24,7 +24,8 @@ internal partial class QuickInfoPresenter : ForegroundThreadAffinitizedObject, I
        private readonly DeferredContentFrameworkElementFactory _elementFactory;

        [ImportingConstructor]
-        public QuickInfoPresenter(IQuickInfoBroker quickInfoBroker, DeferredContentFrameworkElementFactory elementFactory)
+        public QuickInfoPresenter(IThreadingContext threadingContext, IQuickInfoBroker quickInfoBroker, DeferredContentFrameworkElementFactory elementFactory)


Sounds good. Is there an issue tracking future work to update old code?

DustinCampbell · 2018-08-09T15:48:29Z

src/VisualStudio/Core/Def/Implementation/Snippets/AbstractSnippetCommandHandler.cs

@@ -36,7 +34,8 @@ internal abstract class AbstractSnippetCommandHandler :

        public string DisplayName => FeaturesResources.Snippets;

-        public AbstractSnippetCommandHandler(IVsEditorAdaptersFactoryService editorAdaptersFactoryService, SVsServiceProvider serviceProvider)
+        public AbstractSnippetCommandHandler(IThreadingContext threadingContext, IVsEditorAdaptersFactoryService editorAdaptersFactoryService, SVsServiceProvider serviceProvider)


minor: Shouldn't this constructor be protected?

DustinCampbell · 2018-08-09T15:48:48Z

src/VisualStudio/Core/Def/Implementation/Snippets/AbstractSnippetExpansionClient.cs

@@ -46,7 +44,8 @@ internal abstract class AbstractSnippetExpansionClient : ForegroundThreadAffinit

        internal IVsExpansionSession ExpansionSession;

-        public AbstractSnippetExpansionClient(Guid languageServiceGuid, ITextView textView, ITextBuffer subjectBuffer, IVsEditorAdaptersFactoryService editorAdaptersFactoryService)
+        public AbstractSnippetExpansionClient(IThreadingContext threadingContext, Guid languageServiceGuid, ITextView textView, ITextBuffer subjectBuffer, IVsEditorAdaptersFactoryService editorAdaptersFactoryService)


minor: protected constructor?

➡️ dotnet/roslyn-analyzers#1777

DustinCampbell · 2018-08-09T15:49:41Z

src/VisualStudio/Core/Def/Implementation/Snippets/AbstractSnippetInfoService.cs

@@ -43,9 +43,11 @@ internal abstract class AbstractSnippetInfoService : ForegroundThreadAffinitized
        private readonly IAsynchronousOperationListener _waiter;

        public AbstractSnippetInfoService(


minor: should this constructor on an abstract class be protected?

➡️ Also covered by analyzer limitation

DustinCampbell · 2018-08-09T15:55:20Z

src/VisualStudio/Core/Test/Snippets/SnippetTestState.vb

-            Public Sub New(startActiveSession As Boolean)
-                MyBase.New(Nothing, Nothing, Nothing, Nothing)
+            Public Sub New(threadingContext As IThreadingContext, startActiveSession As Boolean)
+                MyBase.New(threadingContext, Nothing, Nothing, Nothing, Nothing)


minor: Named arguments for the trailing Nothing literals would be nice here.

➡️ I would fix this if I was making other changes, but for now I'll consider it covered by #19186

DustinCampbell · 2018-08-09T15:59:38Z

src/Workspaces/MSBuildTest/MSBuildWorkspaceTests.cs

@@ -3252,7 +3252,7 @@ public async Task TestAddRemoveMetadataReference_GAC()

        [ConditionalFact(typeof(VisualStudioMSBuildInstalled))]
        [Trait(Traits.Feature, Traits.Features.MSBuildWorkspace)]
-        public async void TestAddRemoveMetadataReference_ReferenceAssembly()
+        public async Task TestAddRemoveMetadataReference_ReferenceAssembly()


good catch! I missed that one.

sharwell · 2018-08-13T03:33:44Z

@dotnet/roslyn-infrastructure windows_debug_vs-integration_prtest is failing, but not due to this pull request. Merging and will file follow-up issues first thing tomorrow and start a deeper investigation into the failures.

sharwell added 3 commits July 23, 2018 11:56

Enable vs-threading analyzers version 15.8.132

cb37547

Enable VSTHRD100 (Avoid async void methods) and fix violations

cf25962

Enable VSTHRD101 (Avoid unsupported async delegates) and fix violations

fbbf10d

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 23, 2018

sharwell requested a review from a team as a code owner July 23, 2018 16:57

Update to Microsoft.VisualStudio.Threading 15.5.32 for WithPriority(D…

28103a3

…ispatcherPriority)

sharwell requested review from a team as code owners July 23, 2018 18:58

Neme12 reviewed Jul 23, 2018

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into vs-threading-analyzers

1ab9e23

sharwell force-pushed the vs-threading-analyzers branch 4 times, most recently from 4e99053 to fb1e11b Compare July 25, 2018 20:59

Define IThreadingContext and provide it to ForegroundThreadAffinitize…

dc48c9b

…dObject

sharwell force-pushed the vs-threading-analyzers branch from fb1e11b to dc48c9b Compare July 25, 2018 21:39

sharwell added 2 commits July 26, 2018 07:23

Apply MefConstruction.ImportingConstructorMessage to constructors

95032b5

Make LinkedFileUtilities a proper MEF component

059cd2f

sharwell force-pushed the vs-threading-analyzers branch from 2a93c17 to 059cd2f Compare July 26, 2018 23:59

Implement ThreadingContext and remove use of placeholder ThreadingCon…

ac2cfcb

…text.Invalid

sharwell force-pushed the vs-threading-analyzers branch 3 times, most recently from b0cdbbc to 71c6f80 Compare July 28, 2018 15:55

sharwell commented Jul 28, 2018

View reviewed changes

Use JoinableTaskFactory (via IThreadingContext) for switching to the …

2c85db1

…main thread

sharwell force-pushed the vs-threading-analyzers branch 2 times, most recently from 6401c77 to c302b71 Compare July 28, 2018 20:59

Use IThreadingContext.HasMainThread where appropriate

23c08dd

sharwell force-pushed the vs-threading-analyzers branch from c302b71 to 23c08dd Compare July 28, 2018 23:38

CyrusNajmabadi reviewed Aug 7, 2018

View reviewed changes

sharwell force-pushed the vs-threading-analyzers branch from 9d8b071 to bb01d0e Compare August 8, 2018 15:40

jaredpar approved these changes Aug 8, 2018

View reviewed changes

DustinCampbell approved these changes Aug 9, 2018

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into vs-threading-analyzers

dc15c56

sharwell merged commit bd1e0c6 into dotnet:master Aug 13, 2018

sharwell deleted the vs-threading-analyzers branch August 13, 2018 03:34

CyrusNajmabadi mentioned this pull request Aug 23, 2018

CSharpVirtualCharService throwing various asserts #29451

Closed

CyrusNajmabadi mentioned this pull request Nov 2, 2022

IDE Threading improvements #30463

Closed

6 tasks

		{
		await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();

		{
		await threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();

		@@ -43,9 +43,11 @@ internal abstract class AbstractSnippetInfoService : ForegroundThreadAffinitized
		private readonly IAsynchronousOperationListener _waiter;

		public AbstractSnippetInfoService(

Migrate to Microsoft.VisualStudio.Threading #28781

Migrate to Microsoft.VisualStudio.Threading #28781

Conversation

sharwell commented Jul 23, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

jasonmalinowski commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 7, 2018

jasonmalinowski commented Aug 8, 2018

sharwell commented Aug 8, 2018 • edited Loading

jaredpar left a comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Aug 13, 2018

sharwell commented Jul 23, 2018 •

edited

Loading

sharwell commented Aug 8, 2018 •

edited

Loading

sharwell Aug 9, 2018 •

edited

Loading

sharwell Aug 9, 2018 •

edited

Loading