Skip to content

Commit 0a82b93

Browse files
authored
Use correct error response for cancellations (#2939)
### 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.
1 parent d95f64e commit 0a82b93

File tree

3 files changed

+61
-2
lines changed

3 files changed

+61
-2
lines changed

Diff for: lib/ruby_lsp/base_server.rb

+13-2
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,8 @@ def start
9191
# The following requests need to be executed in the main thread directly to avoid concurrency issues. Everything
9292
# else is pushed into the incoming queue
9393
case method
94-
when "initialize", "initialized", "textDocument/didOpen", "textDocument/didClose", "textDocument/didChange"
94+
when "initialize", "initialized", "textDocument/didOpen", "textDocument/didClose", "textDocument/didChange",
95+
"$/cancelRequest"
9596
process_message(message)
9697
when "shutdown"
9798
send_log_message("Shutting down Ruby LSP...")
@@ -133,6 +134,12 @@ def pop_response
133134
@outgoing_queue.pop
134135
end
135136

137+
# This method is only intended to be used in tests! Pushes a message to the incoming queue directly
138+
sig { params(message: T::Hash[Symbol, T.untyped]).void }
139+
def push_message(message)
140+
@incoming_queue << message
141+
end
142+
136143
sig { abstract.params(message: T::Hash[Symbol, T.untyped]).void }
137144
def process_message(message); end
138145

@@ -154,7 +161,11 @@ def new_worker
154161
# Check if the request was cancelled before trying to process it
155162
@mutex.synchronize do
156163
if id && @cancelled_requests.include?(id)
157-
send_message(Result.new(id: id, response: nil))
164+
send_message(Error.new(
165+
id: id,
166+
code: Constant::ErrorCodes::REQUEST_CANCELLED,
167+
message: "Request #{id} was cancelled",
168+
))
158169
@cancelled_requests.delete(id)
159170
next
160171
end

Diff for: lib/ruby_lsp/utils.rb

+3
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,9 @@ class Error
173173
sig { returns(String) }
174174
attr_reader :message
175175

176+
sig { returns(Integer) }
177+
attr_reader :code
178+
176179
sig { params(id: Integer, code: Integer, message: String, data: T.nilable(T::Hash[Symbol, T.untyped])).void }
177180
def initialize(id:, code:, message:, data: nil)
178181
@id = id

Diff for: test/server_test.rb

+45
Original file line numberDiff line numberDiff line change
@@ -744,6 +744,51 @@ def test_requests_to_a_non_existing_position_return_error
744744
assert_match("Request textDocument/completion failed to find the target position.", error.message)
745745
end
746746

747+
def test_cancelling_requests_returns_expected_error_code
748+
uri = URI("file:///foo.rb")
749+
750+
@server.process_message({
751+
method: "textDocument/didOpen",
752+
params: {
753+
textDocument: {
754+
uri: uri,
755+
text: "class Foo\nend",
756+
version: 1,
757+
languageId: "ruby",
758+
},
759+
},
760+
})
761+
762+
mutex = Mutex.new
763+
mutex.lock
764+
765+
# Use a mutex to lock the request in the middle so that we can cancel it before it finishes
766+
@server.stubs(:text_document_definition) do |_message|
767+
mutex.synchronize { 123 }
768+
end
769+
770+
thread = Thread.new do
771+
@server.push_message({
772+
id: 1,
773+
method: "textDocument/definition",
774+
params: {
775+
textDocument: {
776+
uri: uri,
777+
},
778+
position: { line: 0, character: 6 },
779+
},
780+
})
781+
end
782+
783+
@server.process_message({ method: "$/cancelRequest", params: { id: 1 } })
784+
mutex.unlock
785+
thread.join
786+
787+
error = find_message(RubyLsp::Error)
788+
assert_equal(RubyLsp::Constant::ErrorCodes::REQUEST_CANCELLED, error.code)
789+
assert_equal("Request 1 was cancelled", error.message)
790+
end
791+
747792
private
748793

749794
def with_uninstalled_rubocop(&block)

0 commit comments

Comments
 (0)