Skip to content

Commit

Permalink
Use correct error response for cancellations (#2939)
Browse files Browse the repository at this point in the history
### Motivation

While working on #2938, I noticed that our request cancellation response was incorrect. The spec determines that requests that got cancelled by the client need to return an error using the code for request cancelled ([see here](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#cancelRequest) and see `RequestCancelled` [here](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#errorCodes)).

### Implementation

Started returning an error with the right code, rather than a response with a `nil` body.

I also started running cancel notifications directly in the main thread without pushing it to the queue, which was another mistake. If we put that notification in the queue, then we're guaranteed to only process them **after** we finish running requests, which is useless. We need the ability to process them as soon as we receive the notification, so that we have enough time to cancel the requests.

### Automated Tests

Added a test to verify this.
  • Loading branch information
vinistock authored Nov 29, 2024
1 parent d95f64e commit 0a82b93
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 2 deletions.
15 changes: 13 additions & 2 deletions lib/ruby_lsp/base_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ def start
# The following requests need to be executed in the main thread directly to avoid concurrency issues. Everything
# else is pushed into the incoming queue
case method
when "initialize", "initialized", "textDocument/didOpen", "textDocument/didClose", "textDocument/didChange"
when "initialize", "initialized", "textDocument/didOpen", "textDocument/didClose", "textDocument/didChange",
"$/cancelRequest"
process_message(message)
when "shutdown"
send_log_message("Shutting down Ruby LSP...")
Expand Down Expand Up @@ -133,6 +134,12 @@ def pop_response
@outgoing_queue.pop
end

# This method is only intended to be used in tests! Pushes a message to the incoming queue directly
sig { params(message: T::Hash[Symbol, T.untyped]).void }
def push_message(message)
@incoming_queue << message
end

sig { abstract.params(message: T::Hash[Symbol, T.untyped]).void }
def process_message(message); end

Expand All @@ -154,7 +161,11 @@ def new_worker
# Check if the request was cancelled before trying to process it
@mutex.synchronize do
if id && @cancelled_requests.include?(id)
send_message(Result.new(id: id, response: nil))
send_message(Error.new(
id: id,
code: Constant::ErrorCodes::REQUEST_CANCELLED,
message: "Request #{id} was cancelled",
))
@cancelled_requests.delete(id)
next
end
Expand Down
3 changes: 3 additions & 0 deletions lib/ruby_lsp/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ class Error
sig { returns(String) }
attr_reader :message

sig { returns(Integer) }
attr_reader :code

sig { params(id: Integer, code: Integer, message: String, data: T.nilable(T::Hash[Symbol, T.untyped])).void }
def initialize(id:, code:, message:, data: nil)
@id = id
Expand Down
45 changes: 45 additions & 0 deletions test/server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,51 @@ def test_requests_to_a_non_existing_position_return_error
assert_match("Request textDocument/completion failed to find the target position.", error.message)
end

def test_cancelling_requests_returns_expected_error_code
uri = URI("file:///foo.rb")

@server.process_message({
method: "textDocument/didOpen",
params: {
textDocument: {
uri: uri,
text: "class Foo\nend",
version: 1,
languageId: "ruby",
},
},
})

mutex = Mutex.new
mutex.lock

# Use a mutex to lock the request in the middle so that we can cancel it before it finishes
@server.stubs(:text_document_definition) do |_message|
mutex.synchronize { 123 }
end

thread = Thread.new do
@server.push_message({
id: 1,
method: "textDocument/definition",
params: {
textDocument: {
uri: uri,
},
position: { line: 0, character: 6 },
},
})
end

@server.process_message({ method: "$/cancelRequest", params: { id: 1 } })
mutex.unlock
thread.join

error = find_message(RubyLsp::Error)
assert_equal(RubyLsp::Constant::ErrorCodes::REQUEST_CANCELLED, error.code)
assert_equal("Request 1 was cancelled", error.message)
end

private

def with_uninstalled_rubocop(&block)
Expand Down

0 comments on commit 0a82b93

Please sign in to comment.