Skip to content

Completely dettach IO from parallel request execution #368

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

Merged
merged 1 commit into from
Jan 17, 2023
Merged

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Nov 25, 2022

Motivation

Closes #378

Our request Handler class has leaked some responsibility over to the queue and server files. Both request definition and IO are now spread and that makes it difficult to guarantee that no IO will occur in the main request path.

This refactor tries to centralize all request running logic into an Executor class while leaving all queue and IO managing to the Handler. The only part that leaks into the Handler is defining which requests must be run sequentially inside the main process (typically client notifications and life cycle requests).

Implementation

Before

  • All requests are implemented in server.rb
  • Handler and Queue work together to decide how to execute and finalize requests, as well as managing the request queue
  • A bit difficult to display send arbitrary notifications to the client. We currently use the on_error configuration block in server.rb to show error message notifications, but we are unable to send more than one notification to the client (e.g.: we wouldn't be able to clear diagnostics for a file and display a dialogue in the same request)

After

  • The Executor class knows how to run all requests (except for requests that need access to the main process - either the queue or the thread worker). It is basically a big case statement that matches the feature to a method containing the implementation
  • In the Executor, every request returns a response and all notifications (which should not return a response) return Void
  • In addition to request responses, we may want to display notifications to the user. For that, requests can set the @notifications variable, which is an array of notifications to be returned to the client. For example, this allows us to return a response, clear diagnostics and show a dialogue to the user all within the same request
  • The Handler class manages the queue and decides which requests are run sequentially or in parallel. All IO happens here, which should make parallelism a bit easier. We can run requests in separate processes, get the response back and then write to IO

What should reviewers focus on

  • Do you think this new structure is cleaner?
  • Is it easier to understand?
  • How can we better document it in the source code?
  • Do you have a suggestion on how to structure it differently?

Manual tests

Please, take the time to play around with the LSP on this branch to see if you can make it break. I tested on a few projects and things seem to be fine.

@vinistock vinistock added enhancement New feature or request breaking-change Non-backward compatible change labels Nov 25, 2022
@vinistock vinistock self-assigned this Nov 25, 2022
@vinistock vinistock force-pushed the vs/refactor branch 2 times, most recently from 19bcdb2 to 8d166bf Compare December 12, 2022 19:10
@vinistock vinistock force-pushed the vs/refactor branch 2 times, most recently from c11d747 to b20d77d Compare December 15, 2022 19:17
@vinistock vinistock marked this pull request as ready for review December 15, 2022 19:37
@vinistock vinistock requested a review from a team as a code owner December 15, 2022 19:37
@vinistock vinistock force-pushed the vs/refactor branch 2 times, most recently from c4c763d to ffb6968 Compare January 9, 2023 13:54
Copy link
Contributor

@bitwise-aiden bitwise-aiden left a comment

Choose a reason for hiding this comment

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

From someone who is unfamiliar with the history of this project, this change seems to make a lot of sense.

One thing that I do want to ask, is if you'd thought at all about this ticket during the refactor: #443

It feels like they align quite well and at the very least would be worth documenting initial thoughts on how plugins could be added with the new code layout.

Otherwise it's looking good!

@vinistock
Copy link
Member Author

This refactor doesn't take much of the plugin mechanisms into consideration, but I don't think it makes it harder either. The way I'm currently thinking about it, we'll need to allow plugins to define middleware and overrides for requests and those will be loaded at boot.

The plugins shouldn't interact with global store, queues and things like that, they should basically just have access to what the request implementation already has (generally the document and some extra parameters like position).

Copy link
Contributor

@bitwise-aiden bitwise-aiden left a comment

Choose a reason for hiding this comment

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

Awesome work, this is really easy to follow now

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Great job pulling off this huge rewrite. Anyway we can split the changes into multiple PRs/commits so it'd be easier to review?

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

LGTM. Just 2 nitpicks 🙂

Refactor the queue and server so that the handler holds all
responsibility over the job queue and IO
@vinistock vinistock merged commit 71771a5 into main Jan 17, 2023
@vinistock vinistock deleted the vs/refactor branch January 17, 2023 13:07
@shopify-shipit shopify-shipit bot temporarily deployed to production February 15, 2023 20:50 Inactive
klaaspieter pushed a commit to klaaspieter/ruby-lsp that referenced this pull request Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Non-backward compatible change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Queue, Handler and Server to decouple IO from request processing
3 participants