Skip to content

Commit

Permalink
Fail requests that are searching for a non existing position (#2938)
Browse files Browse the repository at this point in the history
### Motivation

This is an attempt to handle and better understand #2446

We're seeing the server get stuck sometimes and I suspect that what's happening is the following:

1. A position request gets triggered, like completion, looking for a certain spot in the code
2. The code changes right as we start locating the position
3. Ruby switches threads and we process the text edit **before** finishing finding the specified position
4. Since now the document is in a different state, that position is no longer valid and it may lead to infinite loops

I propose we start raising when we fail to locate a position in the document and then we fail the request. This will hopefully help us better understand what's happening.

### Implementation

Started raising if the scanner position is already past the document range, which may happen if we modify the document exactly as we're searching for the position.

### Automated Tests

Added tests.
  • Loading branch information
vinistock authored Nov 29, 2024
1 parent b0eb396 commit 0d421a6
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 2 deletions.
11 changes: 10 additions & 1 deletion lib/ruby_lsp/document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class LanguageId < T::Enum
extend T::Helpers
extend T::Generic

class LocationNotFoundError < StandardError; end
ParseResultType = type_member

# This maximum number of characters for providing expensive features, like semantic highlighting and diagnostics.
Expand Down Expand Up @@ -144,7 +145,15 @@ def initialize(source, encoding)
def find_char_position(position)
# Find the character index for the beginning of the requested line
until @current_line == position[:line]
@pos += 1 until LINE_BREAK == @source[@pos]
until LINE_BREAK == @source[@pos]
@pos += 1

if @pos >= @source.length
# Pack the code points back into the original string to provide context in the error message
raise LocationNotFoundError, "Requested position: #{position}\nSource:\n\n#{@source.pack("U*")}"
end
end

@pos += 1
@current_line += 1
end
Expand Down
14 changes: 13 additions & 1 deletion lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,24 @@ def process_message(message)
# 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)
case e
when Store::NonExistingDocumentError
send_message(Error.new(
id: message[:id],
code: Constant::ErrorCodes::INVALID_PARAMS,
message: e.full_message,
))
when Document::LocationNotFoundError
send_message(Error.new(
id: message[:id],
code: Constant::ErrorCodes::REQUEST_FAILED,
message: <<~MESSAGE,
Request #{message[:method]} failed to find the target position.
The file might have been modified while the server was in the middle of searching for the target.
If you experience this regularly, please report any findings and extra information on
https://github.com/Shopify/ruby-lsp/issues/2446
MESSAGE
))
else
send_message(Error.new(
id: message[:id],
Expand Down
23 changes: 23 additions & 0 deletions test/ruby_document_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,29 @@ class Foo
assert_nil(document.cache_get("textDocument/codeLens"))
end

def test_locating_a_non_existing_location_raises
document = RubyLsp::RubyDocument.new(source: <<~RUBY.chomp, version: 1, uri: URI("file:///foo/bar.rb"))
class Foo
end
RUBY

# Exactly at the last character doesn't raise
document.locate_node({ line: 1, character: 2 })

# Anything beyond does
error = assert_raises(RubyLsp::Document::LocationNotFoundError) do
document.locate_node({ line: 3, character: 2 })
end

assert_equal(<<~MESSAGE.chomp, error.message)
Requested position: {:line=>3, :character=>2}
Source:
class Foo
end
MESSAGE
end

private

def assert_error_edit(actual, error_range)
Expand Down
30 changes: 30 additions & 0 deletions test/server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,36 @@ def handle_window_show_message_response(title)
end
end

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

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

@server.process_message({
id: 1,
method: "textDocument/completion",
params: {
textDocument: {
uri: uri,
},
position: { line: 10, character: 0 },
},
})

error = find_message(RubyLsp::Error)
assert_match("Request textDocument/completion failed to find the target position.", error.message)
end

private

def with_uninstalled_rubocop(&block)
Expand Down

0 comments on commit 0d421a6

Please sign in to comment.