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

[WIP] fix: lsp deadlock bug #34

Closed
wants to merge 1 commit into from

Conversation

Shuenhoy
Copy link

@Shuenhoy Shuenhoy commented Aug 15, 2020

There is some deadlock bug in the LSP server, which makes the VSCode extension almost unusable.
In general, the bug happens because a mutex is locked when it is already locked in the same thread like this:

mutex.lock() //A
|a()
|  -> b()
|       -> ...
|            -> mutex.lock()
mutex.unlock() 

In fact, there is no need to hold the mutex lock for such a long time for A. And that is how the bug is fixed.


There seems to still be some problems, I would re-test it.

@Shuenhoy
Copy link
Author

rust-lang/rust#75313 causes the CI build failure.

@Shuenhoy Shuenhoy changed the title fix: lsp deadlock bug [WIP] fix: lsp deadlock bug Aug 15, 2020
@Shuenhoy
Copy link
Author

It seems there are still some times deadlock would happen after the current fix, but I cannot easily reproduce.

@Shuenhoy Shuenhoy changed the title [WIP] fix: lsp deadlock bug fix: lsp deadlock bug Aug 15, 2020
@Shuenhoy
Copy link
Author

There are still some situations, the LSP server would get stuck. However, I can hardly reproduce this. Doing some random typing for a long time sometimes causes this. I am not sure it is the same deadlock issue. Anywhere, after this fix, the LSP server would not be stuck in most situations.

@Shuenhoy Shuenhoy changed the title fix: lsp deadlock bug [WIP] fix: lsp deadlock bug Aug 15, 2020
@Marwes
Copy link
Member

Marwes commented Aug 16, 2020

Thanks for the PR! I believe I made the same fixes in #36 which should have been released (or at least have sat in the PR queue if I hadn't forgotten :( ). I don't remember seeing any lock ups after that, but I haven't had a chance to use that change for to long.

rust-lang/rust#75313 causes the CI build failure.

Nice to see that fixed, I have noticed that issue but after searching through the issue tracker I actually didn't submit a new issue since I saw what I figured were identical reports already.

@Shuenhoy
Copy link
Author

Thanks for the PR! I believe I made the same fixes in #36 which should have been released (or at least have sat in the PR queue if I hadn't forgotten :( ). I don't remember seeing any lock ups after that, but I haven't had a chance to use that change for to long.

rust-lang/rust#75313 causes the CI build failure.

Nice to see that fixed, I have noticed that issue but after searching through the issue tracker I actually didn't submit a new issue since I saw what I figured were identical reports already.

I didn't succeed in finding all the issues, so it's good news that you have fixed it completely, which means there will be a usable LSP. Closing the PR.

@Shuenhoy Shuenhoy closed this Aug 16, 2020
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.

2 participants