Another attempt to move MiscellaneousFilesWorkspace to a bg thread #77768
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This previously failed with integration test failures. I attribute this to previously trying to move the VisualStudioMetadataReferenceManager when that very much wants to be on the main thread. Instead, just ensure that the code that creates the VisualStudioMetadataReferenceManager is on the main thread when that value is needed (it already was). 1) Move MiscellaneousFilesWorkspace to bg thread MEF construction 2) Move HostServicesProvider to location of VisualStudioMefHostServices and use in a couple places (this prevents MiscellaneousFilesWorkspace from needing to MEF import VisualStudioWorkspace in it's ctor)
ghost
added
Area-IDE
untriaged
… unit test changes
Open
3 tasks
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| IMetadataAsSourceFileService fileTrackingMetadataAsSourceService, | ||
| VisualStudioWorkspace visualStudioWorkspace) | ||
| : base(visualStudioWorkspace.Services.HostServices, WorkspaceKind.MiscellaneousFiles) | ||
| Lazy<IMetadataAsSourceFileService> fileTrackingMetadataAsSourceService, |
Member
There was a problem hiding this comment.
Why making the IMetadataAsSourceFileService lazy? Was there some other hidden UI thread dependency there, or it's just a generally big service and there's no reason to ask for it early? A comment might be good either way.
Contributor
Author
There was a problem hiding this comment.
I'd like to flip that question and rather us question why parameters aren't lazy in the package load path. This one switched over to being lazy pretty easily, so I did.
Comment on lines
125
to
126
| // VisualStudioMetadataReferenceManager construction requires the main thread | ||
| _threadingContext.ThrowIfNotOnUIThread(); |
Member
There was a problem hiding this comment.
Do we have a bug tracking fixing this already? It'd be good to link to this to make sure we don't forget to clean it up later.
Contributor
Author
There was a problem hiding this comment.
Went ahead and created something:
Comment on lines
288
to
289
| // Potential calculation of _metadataReferences requires being on the main thread | ||
| _threadingContext.ThrowIfNotOnUIThread(); |
Member
There was a problem hiding this comment.
Same as above: consider adding a comment here to wahtever bug we have tracking cleaning this up.
jasonmalinowski
approved these changes
Mar 24, 2025
ToddGrun
added a commit
that referenced
this pull request
Apr 6, 2025
ToddGrun
added a commit
to ToddGrun/roslyn
that referenced
this pull request
Apr 8, 2025
ToddGrun
added a commit
that referenced
this pull request
Apr 12, 2025
…78051) This PR restores behavior introduced in #77768 that were reverted in PR #77983. The revert occurred as it was flagged for causing ddrit regressions in some of the WinformsVS64.Designer tests. I was unable to reproduce this scenario locally, but the RPS tests are able to reliably hit this issue. Digging into it a bit indicated that creating the OpenTextBufferProvider on a bg thread somehow a thread affinity that caused microsoft.visualstudio.shell.design.ni!DocData.CreateNativeDocData's call to msenv!CEditorManager::RegisterInvisibleEditor's to require an RPC main->bg thread switch. This breaks the assumptions of RegisterInvisibleEditor and weird things start happening. Jason helped track down the likely culprit, and logged https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2441480 to track that work. This PR works around that issue by obtaining the RDT while on the main thread. I've logged #78050 to track the Roslyn cleanup necessary once obtaining the rdt can be done from a background thread. a) Restores changes from PR #77768 that were reverted in PR #77983 b) Ensure OpenTextBufferProvider._runningDocumentTable is initialized on the main thread. c) Added comments about https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2441480 and created a tracking bug for us to clean this up once it's fixed (#78050)
This was referenced Apr 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This previously failed with integration test failures. I attribute this to previously trying to move the VisualStudioMetadataReferenceManager to bg thread construction when that very much wants to be on the main thread. Instead, just ensure that the code that creates the VisualStudioMetadataReferenceManager is on the main thread when that value is needed (it already was).