Migrate to Microsoft.VisualStudio.Threading by sharwell · Pull Request #28781 · dotnet/roslyn

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

                _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

Sign in to view

…main thread

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

                        },
-                        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

                    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

            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

                _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

                _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


        [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

        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

        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

@@ -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

-            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

        [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

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

		public AbstractSnippetInfoService(

Conversation

sharwell commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

jasonmalinowski commented Jul 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 7, 2018

Uh oh!

jasonmalinowski commented Aug 8, 2018

Uh oh!

sharwell commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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