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

Synchronize didOpen and didClose notifications #1901

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

vinistock
Copy link
Member

Motivation

Closes #1897

In the server / base server refactor, we stopped running didOpen and didClose under a mutex lock, which is not correct.

If we don't run it under a mutex lock, we risk having threads switch and trying to execute a different request before a file has been saved into the Store state, which will then try to find the document in the store, but won't find it.

For files that haven't been saved yet (untitled:Untitled-1 URIs), that will end up trying to read from disk, which is guaranteed to fail since the file wasn't saved.

Implementation

We need to run didOpen and didClose under a mutex lock. We never want the threads to switch during those notifications.

Automated Tests

It's quite hard to write a test for this, so I haven't added one.

Manual Tests

Manually testing is quite easy.

  1. Start the LSP on this branch
  2. Create an unsaved Ruby file
  3. Now, with the unsaved Ruby file selected, restart the LSP
  4. Verify it doesn't break

@vinistock vinistock added bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes labels Apr 8, 2024
@vinistock vinistock self-assigned this Apr 8, 2024
@vinistock vinistock requested a review from a team as a code owner April 8, 2024 20:28
@vinistock vinistock force-pushed the vs/synchronize_did_open_and_close branch from 6389345 to 8a671fe Compare April 8, 2024 20:29
@Earlopain
Copy link
Contributor

The repro seems different for me, I can only get it to (not) work with an extra step, can you check if this PR also fixes this? I still get the error on this branch checked out with the steps below: (maybe I'm doing something wrong)

  1. Start the LSP on this branch
  2. Create an unsaved Ruby file
  3. Switch to a saved Ruby file
  4. Now, with the saved Ruby file selected, restart the LSP
  5. Verify it doesn't break

@vinistock
Copy link
Member Author

I believe it should fix that scenario as well. Editors will first send a didOpen notification for each file opened in the UI and then fire other feature requests. Since we're running all didOpen notifications under a lock, the server won't be able to switch threads until they are all processed.

@vinistock vinistock merged commit 729df3e into main Apr 8, 2024
39 checks passed
@vinistock vinistock deleted the vs/synchronize_did_open_and_close branch April 8, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

store.rb:48:in `binread': no implicit conversion of nil into String (TypeError)
3 participants