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

Revert "lsp: Parse LSP messages on background thread - again (#23122)" #23301

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

ConradIrwin
Copy link
Member

This reverts commit 1b3b825.

When debugging git diffs we found that this introduced a re-ordering of messages sent to the LSP:

  • User hits "format"
  • Zed adjusts spacing, and sends "spaces changed" to the LSP
  • Zed sends "format" to LSP

With the async approach here, the format request can now arrive before the space changed request.

You can reproduce this with test_strip_whitespace_and_format_via_lsp under some conditions.

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 17, 2025
@ConradIrwin ConradIrwin merged commit a247617 into main Jan 17, 2025
12 checks passed
@ConradIrwin ConradIrwin deleted the revert-lsp-change branch January 17, 2025 22:06
osiewicz added a commit that referenced this pull request Jan 17, 2025
osiewicz added a commit that referenced this pull request Jan 17, 2025
As @ConradIrwin found out in #23301 the LSP messages could be processed out of order after #23122; this PR makes our
approach to event processing a bit more conservative - instead of detaching the parsing task we'll now await them in the main loop.
This keeps the benefit of #23122 in that deserialization still happens off the main thread, but some of the performance benefit of #23122 could've been a result of the buggy interaction
that Conrad discovered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant