-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
fix(lsp): re-enable the per resource configuration without a deadlock #10625
Conversation
d43164c
to
adabd40
Compare
I've added a benchmark test that replicates the issue that I encountered which required the feature to be removed. I thought about just leaving it as a integration test, but it is a good test to have additional benchmarks for the lsp and it is significantly different than the other ones we have. |
479a960
to
0114a3a
Compare
0114a3a
to
947e09e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed lightly due to time pressure but LGTM.
cli/lsp/config.rs
Outdated
if self.client_capabilities.workspace_configuration | ||
&& !self | ||
.settings | ||
.try_read() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would you expect this to fail?
I'd probably just send unconditionally, without checking .specifiers first, and let the receiver handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, when I was pondering this offline I came to the same thought, it would put an extra message in the queue, but it would get cleared down in a much narrower lock situation.
947e09e
to
d793599
Compare
This PR rewrites the way per resource configuration is handled in the language server.
There isn't specifically a test that tests for this deadlock yet, as I need to refactor the language server integration tests to a new structure that supports 2 way communication, which we have in the benchmark harness, but need to share with the integration tests.