Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid inlining ILanguageClientBroker initialization #65929

Merged
merged 1 commit into from
Dec 12, 2022
Merged

Avoid inlining ILanguageClientBroker initialization #65929

merged 1 commit into from
Dec 12, 2022

Conversation

davkean
Copy link
Member

@davkean davkean commented Dec 12, 2022 •
edited by Pilchie
Loading

Fixes: AB#1698422

StartListening is called on thread pool, which causes LanguageClientBroker to be initialized inline without yielding and gets indirectly blocked on by the UI thread via a JTF.Run. Instead, yield so that StartListening continues without blocking on the initialization.

Example call stack (note this is two threads stitched together using the background stacks feature):

...
mscorlib.dll!System.Lazy`[System.__Canon].CreateValue
mscorlib.dll!System.Lazy`[System.__Canon].LazyInitValue
microsoft.codeanalysis.editorfeatures.dll!Microsoft.CodeAnalysis.Editor.Implementation.LanguageClient.AlwaysActiveLanguageClientEventListener+d__.MoveNext
?!System.Runtime.CompilerServices.AsyncTaskMethodBuilder.Start
microsoft.codeanalysis.editorfeatures.dll!Microsoft.CodeAnalysis.Editor.Implementation.LanguageClient.AlwaysActiveLanguageClientEventListener.LoadAsync
microsoft.codeanalysis.workspaces.dll!Microsoft.CodeAnalysis.Host.DefaultWorkspaceEventListenerServiceFactory+Service.EnsureListeners
microsoft.codeanalysis.workspaces.dll!Microsoft.CodeAnalysis.Workspace.GetEventHandlers[System.__Canon]
microsoft.codeanalysis.workspaces.dll!Microsoft.CodeAnalysis.Workspace.RaiseWorkspaceChangedEventAsync
microsoft.codeanalysis.workspaces.dll!Microsoft.CodeAnalysis.Workspace.OnSolutionAdded
microsoft.visualstudio.languageservices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioProjectFactory+<>c__DisplayClass_0.b__
microsoft.visualstudio.languageservices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioWorkspaceImpl+d__.MoveNext
microsoft.visualstudio.languageservices.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder.Start[Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioWorkspaceImpl+d__]
microsoft.visualstudio.languageservices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioWorkspaceImpl.ApplyChangeToWorkspaceAsync
microsoft.visualstudio.languageservices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioProjectFactory+d__.MoveNext
mscorlib.dll!System.Threading.ExecutionContext.RunInternal
mscorlib.dll!System.Threading.ExecutionContext.Run
mscorlib.dll!System.Runtime.CompilerServices.AsyncMethodBuilderCore+MoveNextRunner.Run
mscorlib.dll!System.Threading.QueueUserWorkItemCallback.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem
mscorlib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch
clr.dll!CallDescrWorkerInternal
clr.dll!CallDescrWorkerWithHandler
clr.dll!MethodDescCallSite::CallTargetWorker
clr.dll!QueueUserWorkItemManagedCallback
clr.dll!ManagedThreadBase_DispatchInner
clr.dll!ManagedThreadBase_DispatchMiddle
clr.dll!ManagedThreadBase_DispatchOuter
clr.dll!ManagedThreadBase_FullTransitionWithAD
clr.dll!ManagedPerAppDomainTPCount::DispatchWorkItem
clr.dll!ThreadpoolMgr::ExecuteWorkRequest
clr.dll!ThreadpoolMgr::WorkerThreadStart
clr.dll!Thread::intermediateThreadProc
kernel32.dll!BaseThreadInitThunk
ntdll.dll!RtlUserThreadStart
PSEUDOFRAME!SynchronousWaitOnABackgroundThread <!---  THREAD TRANSITION
kernelbase.dll!WaitForMultipleObjectsEx
kernelbase.dll!WaitForMultipleObjects
microsoft.visualstudio.threading.dll!DomainBoundILStubClass.IL_STUB_PInvoke
microsoft.visualstudio.threading.dll!Microsoft.VisualStudio.Threading.NoMessagePumpSyncContext.Wait
clr.dll!CallDescrWorkerInternal
clr.dll!CallDescrWorkerWithHandler
clr.dll!MethodDescCallSite::CallTargetWorker
clr.dll!Thread::DoSyncContextWait
clr.dll!Thread::DoAppropriateWaitWorker
clr.dll!Thread::DoAppropriateWait
clr.dll!CLREventBase::WaitEx
clr.dll!Thread::Block
clr.dll!SyncBlock::Wait
clr.dll!ObjectNative::WaitTimeout
mscorlib.dll!System.Threading.ManualResetEventSlim.Wait
mscorlib.dll!System.Threading.Tasks.Task.SpinThenBlockingWait
mscorlib.dll!System.Threading.Tasks.Task.InternalWait
mscorlib.dll!System.Threading.Tasks.Task.Wait
mscorlib.dll!System.Threading.Tasks.Task.Wait
microsoft.visualstudio.threading.dll!Microsoft.VisualStudio.Threading.JoinableTaskFactory.WaitSynchronouslyCore
microsoft.visualstudio.threading.dll!Microsoft.VisualStudio.Threading.JoinableTaskFactory.WaitSynchronously
microsoft.visualstudio.threading.dll!Microsoft.VisualStudio.Threading.JoinableTask.CompleteOnCurrentThread
microsoft.visualstudio.threading.dll!Microsoft.VisualStudio.Threading.JoinableTask`[System.__Canon].CompleteOnCurrentThread
microsoft.visualstudio.languageservices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.Legacy.AbstractLegacyProject..ctor
microsoft.visualstudio.languageservices.csharp.dll!Microsoft.VisualStudio.LanguageServices.CSharp.ProjectSystemShim.CSharpProjectShim..ctor
microsoft.visualstudio.languageservices.csharp.dll!Microsoft.VisualStudio.LanguageServices.CSharp.LanguageService.CSharpLanguageService.BindToProject
microsoft.visualstudio.languageservices.csharp.dll!dynamicClass.IL_STUB_COMtoCLR
clr.dll!COMToCLRDispatchHelper
clr.dll!COMToCLRWorker
clr.dll!GenericComCallStub
csproj.dll!CCSharpBuildMgrSite::BindToHost
csproj.dll!CCSharpBuildCompiler::SetupEmptyCompiler
csproj.dll!CLangProject::PrepareCompiler
csproj.dll!CVsProject::PrepareCompiler
csproj.dll!CLangProject::OnAfterCreateProject
csproj.dll!CVsProject::OnAfterCreateProject
csproj.dll!CPrefetchedProject::FinalizeProject
csproj.dll!CVsProjPackage::FinalizeProject
?!dynamicClass.IL_STUB_CLRtoCOM

@davkean
Avoid inlining ILanguageClientBroker initialization
395ac99 
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1698422

StartListening is called on thread pool, which causes LanguageClientBroker to be initialized inline without yielding and gets indirectly blocked on by the UI thread. Instead, yield so that StartListening continues without blocking on the initialization.
@davkean davkean requested a review from a team as a code owner December 12, 2022 03:27
CyrusNajmabadi
CyrusNajmabadi reviewed Dec 12, 2022
View reviewed changes
src/EditorFeatures/Core/LanguageServer/AlwaysActiveLanguageClientEventListener.cs
await TaskScheduler.Default;
// doesn't block the UI thread. Note, we always yield because sometimes our caller starts
// on the threadpool thread but is indirectly blocked on by the UI thread.
await TaskScheduler.Default.SwitchTo(alwaysYield: true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharwell should we just make this code the pattern we always use? (and/or have a helper we ourselves use that always does this). I'd like to ensure we aren't just patching this same issue oover and over again in multiple places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm not sure why we didn't just Task.Run() in the StartListening call here...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just make this code the pattern we always use

No, that would be a waste. await TaskScheduler.Default is a safe and clear way to ensure the asynchronous operation is on a background thread, and NOP if you already are.

@CyrusNajmabadi CyrusNajmabadi merged commit 4bf4497 into dotnet:main Dec 12, 2022
@ghost ghost added this to the Next milestone Dec 12, 2022
jasonmalinowski
jasonmalinowski reviewed Dec 12, 2022
View reviewed changes
src/EditorFeatures/Core/LanguageServer/AlwaysActiveLanguageClientEventListener.cs
Comment on lines +63 to +64
// doesn't block the UI thread. Note, we always yield because sometimes our caller starts
// on the threadpool thread but is indirectly blocked on by the UI thread.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to put a comment here explaining this case a bit more since this is otherwise very not obvious without looking at this bug.

src/EditorFeatures/Core/LanguageServer/AlwaysActiveLanguageClientEventListener.cs
await TaskScheduler.Default;
// doesn't block the UI thread. Note, we always yield because sometimes our caller starts
// on the threadpool thread but is indirectly blocked on by the UI thread.
await TaskScheduler.Default.SwitchTo(alwaysYield: true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm not sure why we didn't just Task.Run() in the StartListening call here...

sharwell
sharwell reviewed Dec 12, 2022
View reviewed changes
src/EditorFeatures/Core/LanguageServer/AlwaysActiveLanguageClientEventListener.cs
Comment on lines +63 to +64
// doesn't block the UI thread. Note, we always yield because sometimes our caller starts
// on the threadpool thread but is indirectly blocked on by the UI thread.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that the caller is indirectly blocking the UI thread is somewhat orthogonal to the goal here. We always yield because this method is designed to always be non-blocking, regardless of the calling scenario.

@CyrusNajmabadi
Copy link
Member

my general issue is that there doesn't seem to be a way to sufficiently audit these. we're effectively beholden to dave noticing something and reporting an issue.

@allisonchou allisonchou mentioned this pull request Dec 15, 2022
Closed
@Cosifne Cosifne modified the milestones: Next, 17.5 P3 Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

@sharwell sharwell sharwell left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
17.5 P3
Development

Successfully merging this pull request may close these issues.
5 participants
@davkean @CyrusNajmabadi @jasonmalinowski @sharwell @Cosifne