Skip to content

Ensure we compute LSP server capabilities on UI thread before load#51295

Merged
jasonmalinowski merged 1 commit intodotnet:release/dev16.9-vs-depsfrom
jasonmalinowski:ensure-option-persisters-loaded-during-lsp-initialization
Feb 24, 2021
Merged

Ensure we compute LSP server capabilities on UI thread before load#51295
jasonmalinowski merged 1 commit intodotnet:release/dev16.9-vs-depsfrom
jasonmalinowski:ensure-option-persisters-loaded-during-lsp-initialization

Conversation

@jasonmalinowski
Copy link
Member

"Fixes" https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1261421 in the sense it's a complete and total tactical fix. For 16.9 I think this is the lowest-risk thing we can do to address it, and we'll do the better fixes in 16.10.

This is directly targeting releases/dev16.9-vs-deps because targeting releases/dev16.9 runs into a bunch of merge conflicts; once we ship 16.9 we'll merge this back into releases/dev16.9 and then the fix will be in master where we can do the further cleanups.

@jasonmalinowski jasonmalinowski requested a review from a team as a code owner February 18, 2021 00:17
@ghost ghost added the Area-IDE label Feb 18, 2021
@jasonmalinowski jasonmalinowski self-assigned this Feb 18, 2021
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

:(

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 potentially switch back off the UI thread here, so that the creation of the connection, instantiating the LSP server can happen off of the uI thread?

Copy link
Member Author

Choose a reason for hiding this comment

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

So it looks like this API was already being called on the UI thread; I agree conceptually that it's the right thing to do, but worried about added risk. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, if it's already on it then I'm OK with this - switching off would then be a new path (and potential for bugs).

@jasonmalinowski jasonmalinowski force-pushed the ensure-option-persisters-loaded-during-lsp-initialization branch from 17ee348 to 807b944 Compare February 18, 2021 01:13
@jasonmalinowski jasonmalinowski merged commit db94f4c into dotnet:release/dev16.9-vs-deps Feb 24, 2021
@jasonmalinowski jasonmalinowski deleted the ensure-option-persisters-loaded-during-lsp-initialization branch February 24, 2021 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants