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.

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): remove code_action/diagnostics deadlock #10555

Merged
merged 2 commits into from
May 11, 2021

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented May 10, 2021

Fixes: #10368

This PR refactors the whole of lsp/diagnostics.rs to clean it up as well as remove situations where the language server can end up in a deadlock while processing code actions. Code actions require access to the current TypeScript diagnostics for a given file to determine if there are multiple fixable diagnostics it can perform for a given action. Previously when the language server was running slow (a slow machine or complex TypeScript project) it could end up in a situation where there was a request to update the diagnostics locking the diagnostic collection at the same time a code action was trying to acquire a lock to read the diagnostics and this would "hang" the language server.

There is no easy test for this, but the symptom was actually evident in the tests when the refactor "broke" the test_code_actions. The request for the code_actions would be blocked until the diagnostic refresh completed, which then ran a risk that we would get into a deadlock state if another refresh were to start while the code actions were being processed. A delay now has to be added to ensure the diagnostics are updated as an update to diagnostics is now non-blocking for code_actions which is the way it should have always been.

@kitsonk kitsonk force-pushed the fix_diagnotic_collection_deadlock branch from ef7c969 to 6145670 Compare May 10, 2021 20:12
@kitsonk kitsonk force-pushed the fix_diagnotic_collection_deadlock branch from 6145670 to 2ceb69a Compare May 11, 2021 05:11
@ry ry added this to the 1.10.0 milestone May 11, 2021
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM modulo the DiagnosticRequest::Update -> ()

This PR should have a test, but it's not yet complete. However we want to get this in for 1.10, therefore I've created #10587 to provide a test.

@ry ry merged commit 41a3b17 into denoland:main May 11, 2021
ry added a commit to ry/deno that referenced this pull request May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deno lsp showing false errors and stops linting new code, requires reloading window to resolve
2 participants