From f4f37b46b7e4ba958bf7e215f5f4cacbecad664d Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 22 Feb 2024 10:35:06 -0500 Subject: [PATCH] Fallback to `NullClient` if initializing server fails (#266) * Fallback to `NullClient` if initializing server fails * Fail gracefully on server initialization error --- lib/ruby_lsp/ruby_lsp_rails/addon.rb | 8 +- lib/ruby_lsp/ruby_lsp_rails/runner_client.rb | 80 ++++++++++++++++-- lib/ruby_lsp/ruby_lsp_rails/server.rb | 86 ++++++++++++-------- test/ruby_lsp_rails/hover_test.rb | 24 +++--- test/ruby_lsp_rails/runner_client_test.rb | 18 +++- test/ruby_lsp_rails/server_test.rb | 8 +- 6 files changed, 165 insertions(+), 59 deletions(-) diff --git a/lib/ruby_lsp/ruby_lsp_rails/addon.rb b/lib/ruby_lsp/ruby_lsp_rails/addon.rb index 9dcb828c..755aca84 100644 --- a/lib/ruby_lsp/ruby_lsp_rails/addon.rb +++ b/lib/ruby_lsp/ruby_lsp_rails/addon.rb @@ -14,11 +14,15 @@ class Addon < ::RubyLsp::Addon sig { returns(RunnerClient) } def client - @client ||= T.let(RunnerClient.new, T.nilable(RunnerClient)) + @client ||= T.let(RunnerClient.create_client, T.nilable(RunnerClient)) end sig { override.params(message_queue: Thread::Queue).void } - def activate(message_queue); end + def activate(message_queue) + # Eagerly initialize the client in a thread. This allows the indexing from the Ruby LSP to continue running even + # while we boot large Rails applications in the background + Thread.new { client } + end sig { override.void } def deactivate diff --git a/lib/ruby_lsp/ruby_lsp_rails/runner_client.rb b/lib/ruby_lsp/ruby_lsp_rails/runner_client.rb index fd66a541..f8ae9650 100644 --- a/lib/ruby_lsp/ruby_lsp_rails/runner_client.rb +++ b/lib/ruby_lsp/ruby_lsp_rails/runner_client.rb @@ -4,17 +4,38 @@ require "json" require "open3" -# NOTE: We should avoid printing to stderr since it causes problems. We never read the standard error pipe -# from the client, so it will become full and eventually hang or crash. -# Instead, return a response with an `error` key. - module RubyLsp module Rails class RunnerClient + class << self + extend T::Sig + + sig { returns(RunnerClient) } + def create_client + new + rescue Errno::ENOENT, StandardError => e # rubocop:disable Lint/ShadowedException + warn("Ruby LSP Rails failed to initialize server: #{e.message}\n#{e.backtrace&.join("\n")}") + warn("Server dependent features will not be available") + NullClient.new + end + end + + class InitializationError < StandardError; end + class IncompleteMessageError < StandardError; end + extend T::Sig sig { void } def initialize + # Spring needs a Process session ID. It uses this ID to "attach" itself to the parent process, so that when the + # parent ends, the spring process ends as well. If this is not set, Spring will throw an error while trying to + # set its own session ID + begin + Process.setsid + rescue Errno::EPERM + # If we can't set the session ID, continue + end + stdin, stdout, stderr, wait_thread = Open3.popen3( "bin/rails", "runner", @@ -27,11 +48,20 @@ def initialize @wait_thread = T.let(wait_thread, Process::Waiter) @stdin.binmode # for Windows compatibility @stdout.binmode # for Windows compatibility + + warn("Ruby LSP Rails booting server") + read_response + warn("Finished booting Ruby LSP Rails server") + rescue Errno::EPIPE, IncompleteMessageError + raise InitializationError, @stderr.read end sig { params(name: String).returns(T.nilable(T::Hash[Symbol, T.untyped])) } def model(name) make_request("model", name: name) + rescue IncompleteMessageError + warn("Ruby LSP Rails failed to get model information: #{@stderr.read}") + nil end sig { void } @@ -48,13 +78,18 @@ def stopped? private - sig { params(request: T.untyped, params: T.untyped).returns(T.untyped) } + sig do + params( + request: String, + params: T.nilable(T::Hash[Symbol, T.untyped]), + ).returns(T.nilable(T::Hash[Symbol, T.untyped])) + end def make_request(request, params = nil) send_message(request, params) read_response end - sig { params(request: T.untyped, params: T.untyped).void } + sig { params(request: String, params: T.nilable(T::Hash[Symbol, T.untyped])).void } def send_message(request, params = nil) message = { method: request } message[:params] = params if params @@ -68,8 +103,9 @@ def send_message(request, params = nil) sig { returns(T.nilable(T::Hash[Symbol, T.untyped])) } def read_response headers = @stdout.gets("\r\n\r\n") - raw_response = @stdout.read(T.must(headers)[/Content-Length: (\d+)/i, 1].to_i) + raise IncompleteMessageError unless headers + raw_response = @stdout.read(headers[/Content-Length: (\d+)/i, 1].to_i) response = JSON.parse(T.must(raw_response), symbolize_names: true) if response[:error] @@ -80,5 +116,35 @@ def read_response response.fetch(:result) end end + + class NullClient < RunnerClient + extend T::Sig + + sig { void } + def initialize # rubocop:disable Lint/MissingSuper + end + + sig { override.void } + def shutdown + # no-op + end + + sig { override.returns(T::Boolean) } + def stopped? + true + end + + private + + sig { override.params(request: String, params: T.nilable(T::Hash[Symbol, T.untyped])).void } + def send_message(request, params = nil) + # no-op + end + + sig { override.returns(T.nilable(T::Hash[Symbol, T.untyped])) } + def read_response + # no-op + end + end end end diff --git a/lib/ruby_lsp/ruby_lsp_rails/server.rb b/lib/ruby_lsp/ruby_lsp_rails/server.rb index d5c6a6d4..6b8d14af 100644 --- a/lib/ruby_lsp/ruby_lsp_rails/server.rb +++ b/lib/ruby_lsp/ruby_lsp_rails/server.rb @@ -19,6 +19,9 @@ nil end +# NOTE: We should avoid printing to stderr since it causes problems. We never read the standard error pipe from the +# client, so it will become full and eventually hang or crash. Instead, return a response with an `error` key. + module RubyLsp module Rails class Server @@ -26,6 +29,53 @@ class Server extend T::Sig + sig { void } + def initialize + $stdin.sync = true + $stdout.sync = true + @running = T.let(true, T::Boolean) + end + + sig { void } + def start + initialize_result = { result: { message: "ok" } }.to_json + $stdout.write("Content-Length: #{initialize_result.length}\r\n\r\n#{initialize_result}") + + while @running + headers = $stdin.gets("\r\n\r\n") + json = $stdin.read(headers[/Content-Length: (\d+)/i, 1].to_i) + + request = JSON.parse(json, symbolize_names: true) + response = execute(request.fetch(:method), request[:params]) + next if response == VOID + + json_response = response.to_json + $stdout.write("Content-Length: #{json_response.length}\r\n\r\n#{json_response}") + end + end + + sig do + params( + request: String, + params: T::Hash[Symbol, T.untyped], + ).returns(T.any(Object, T::Hash[Symbol, T.untyped])) + end + def execute(request, params = {}) + case request + when "shutdown" + @running = false + VOID + when "model" + resolve_database_info_from_model(params.fetch(:name)) + else + VOID + end + rescue => e + { error: e.full_message(highlight: false) } + end + + private + sig { params(model_name: String).returns(T::Hash[Symbol, T.untyped]) } def resolve_database_info_from_model(model_name) const = ActiveSupport::Inflector.safe_constantize(model_name) @@ -48,41 +98,7 @@ def resolve_database_info_from_model(model_name) end info rescue => e - { - error: e.message, - } - end - - sig { void } - def start - $stdin.sync = true - $stdout.sync = true - - running = T.let(true, T::Boolean) - - while running - headers = $stdin.gets("\r\n\r\n") - request = $stdin.read(headers[/Content-Length: (\d+)/i, 1].to_i) - - json = JSON.parse(request, symbolize_names: true) - request_method = json.fetch(:method) - params = json[:params] - - response = case request_method - when "shutdown" - running = false - VOID - when "model" - resolve_database_info_from_model(params.fetch(:name)) - else - VOID - end - - next if response == VOID - - json_response = response.to_json - $stdout.write("Content-Length: #{json_response.length}\r\n\r\n#{json_response}") - end + { error: e.full_message(highlight: false) } end end end diff --git a/test/ruby_lsp_rails/hover_test.rb b/test/ruby_lsp_rails/hover_test.rb index 5fa6a7d1..981e1c4d 100644 --- a/test/ruby_lsp_rails/hover_test.rb +++ b/test/ruby_lsp_rails/hover_test.rb @@ -194,18 +194,22 @@ def hover_on_source(source, position) executor.instance_variable_get(:@index).index_single( RubyIndexer::IndexablePath.new(nil, T.must(uri.to_standardized_path)), source ) - response = executor.execute( - { - method: "textDocument/hover", - params: { - textDocument: { uri: uri }, - position: position, + + response = T.let(nil, T.nilable(RubyLsp::Result)) + capture_io do + response = executor.execute( + { + method: "textDocument/hover", + params: { + textDocument: { uri: uri }, + position: position, + }, }, - }, - ) + ) + end - assert_nil(response.error) - response.response + assert_nil(T.must(response).error) + T.must(response).response end def dummy_root diff --git a/test/ruby_lsp_rails/runner_client_test.rb b/test/ruby_lsp_rails/runner_client_test.rb index 23885c2e..022762cf 100644 --- a/test/ruby_lsp_rails/runner_client_test.rb +++ b/test/ruby_lsp_rails/runner_client_test.rb @@ -8,7 +8,9 @@ module RubyLsp module Rails class RunnerClientTest < ActiveSupport::TestCase setup do - @client = T.let(RunnerClient.new, RunnerClient) + capture_io do + @client = T.let(RunnerClient.new, RunnerClient) + end end teardown do @@ -36,6 +38,20 @@ class RunnerClientTest < ActiveSupport::TestCase test "returns nil if the request returns a nil response" do assert_nil @client.model("ApplicationRecord") # ApplicationRecord is abstract end + + test "failing to spawn server creates a null client" do + FileUtils.mv("bin/rails", "bin/rails_backup") + + assert_output("", %r{No such file or directory - bin/rails}) do + client = RunnerClient.create_client + + assert_instance_of(NullClient, client) + assert_nil(client.model("User")) + assert_predicate(client, :stopped?) + end + ensure + FileUtils.mv("bin/rails_backup", "bin/rails") + end end end end diff --git a/test/ruby_lsp_rails/server_test.rb b/test/ruby_lsp_rails/server_test.rb index a9861342..d465f012 100644 --- a/test/ruby_lsp_rails/server_test.rb +++ b/test/ruby_lsp_rails/server_test.rb @@ -10,24 +10,24 @@ class ServerTest < ActiveSupport::TestCase end test "returns nil if model doesn't exist" do - response = @server.resolve_database_info_from_model("Foo") + response = @server.execute("model", { name: "Foo" }) assert_nil(response.fetch(:result)) end test "returns nil if class is not a model" do - response = @server.resolve_database_info_from_model("Time") + response = @server.execute("model", { name: "Time" }) assert_nil(response.fetch(:result)) end test "returns nil if class is an abstract model" do - response = @server.resolve_database_info_from_model("ApplicationRecord") + response = @server.execute("model", { name: "ApplicationRecord" }) assert_nil(response.fetch(:result)) end test "handles older Rails version which don't have `schema_dump_path`" do ActiveRecord::Tasks::DatabaseTasks.send(:alias_method, :old_schema_dump_path, :schema_dump_path) ActiveRecord::Tasks::DatabaseTasks.undef_method(:schema_dump_path) - response = @server.resolve_database_info_from_model("User") + response = @server.execute("model", { name: "User" }) assert_nil(response.fetch(:result)[:schema_file]) ensure ActiveRecord::Tasks::DatabaseTasks.send(:alias_method, :schema_dump_path, :old_schema_dump_path)