Skip to content

Commit

Permalink
Handle documents being deleted before processing their requests
Browse files Browse the repository at this point in the history
  • Loading branch information
vinistock committed Jul 16, 2024
1 parent b6e86b2 commit 0de178a
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 10 deletions.
31 changes: 21 additions & 10 deletions lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,27 @@ def process_message(message)
rescue StandardError, LoadError => e
# If an error occurred in a request, we have to return an error response or else the editor will hang
if message[:id]
send_message(Error.new(
id: message[:id],
code: Constant::ErrorCodes::INTERNAL_ERROR,
message: e.full_message,
data: {
errorClass: e.class.name,
errorMessage: e.message,
backtrace: e.backtrace&.join("\n"),
},
))
# If a document is deleted before we are able to process all of its enqueued requests, we will try to read it
# from disk and it raise this error. This is expected, so we don't include the `data` attribute to avoid
# reporting these to our telemetry
if e.is_a?(Store::NonExistingDocumentError)
send_message(Error.new(
id: message[:id],
code: Constant::ErrorCodes::INVALID_PARAMS,
message: e.full_message,
))
else
send_message(Error.new(
id: message[:id],
code: Constant::ErrorCodes::INTERNAL_ERROR,
message: e.full_message,
data: {
errorClass: e.class.name,
errorMessage: e.message,
backtrace: e.backtrace&.join("\n"),
},
))
end
end

$stderr.puts("Error processing #{message[:method]}: #{e.full_message}")
Expand Down
4 changes: 4 additions & 0 deletions lib/ruby_lsp/store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module RubyLsp
class Store
extend T::Sig

class NonExistingDocumentError < StandardError; end

sig { returns(T::Boolean) }
attr_accessor :supports_progress

Expand Down Expand Up @@ -45,6 +47,8 @@ def get(uri)
end
set(uri: uri, source: File.binread(path), version: 0, language_id: language_id)
T.must(@state[uri.to_s])
rescue Errno::ENOENT
raise NonExistingDocumentError, uri.to_s
end

sig do
Expand Down
59 changes: 59 additions & 0 deletions test/server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,65 @@ def test_handles_editor_indexing_settings
assert_includes(RubyIndexer.configuration.instance_variable_get(:@included_gems), "bar_gem")
end

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

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

# Close the file in a thread to increase the chance that it gets closed during the processing of the 10 document
# symbol requests below
thread = Thread.new do
@server.process_message({
method: "textDocument/didClose",
params: {
textDocument: {
uri: uri,
},
},
})
end

10.times do |i|
@server.process_message({
id: i,
method: "textDocument/documentSymbol",
params: {
textDocument: {
uri: uri,
},
},
})
end

thread.join
end

# Even if the thread technique, this test is not 100% reliable since it's trying to emulate a concurrency issue. If
# we tried to always expect an error back, we would likely get infinite loops
error = T.let(nil, T.nilable(T.any(RubyLsp::Error, RubyLsp::Message)))

10.times do
error = @server.pop_response
break if error.is_a?(RubyLsp::Error)
end

if error.is_a?(RubyLsp::Error)
assert_instance_of(RubyLsp::Error, error)
assert_match("file:///foo.rb (RubyLsp::Store::NonExistingDocumentError)", error.message)
end
end

private

def with_uninstalled_rubocop(&block)
Expand Down

0 comments on commit 0de178a

Please sign in to comment.