Skip to content

Commit

Permalink
Use unique IDs for registering watchers (#3144)
Browse files Browse the repository at this point in the history
### Motivation

Closes #3137

We were using the same registration ID for two different file watchers. That made VS Code override the entry, but one of them became a dangling object that didn't get appropriately cleaned up when the LSP shuts down.

Because that old file watcher was still active, the LSP client itself was not cleared either as a reference was still being held and then notifications for `didChangeWatchedFiles` were sent to a client that was already stopped, resulting in the `Notify file events failed` message.

We should always use unique IDs when registering file watchers.

### Implementation

I did 3 small things:

1. Ensured unique IDs for registering the watchers
2. Started watching `.rubocop`, which we watched before but I missed in my refactor PR
3. Started ensuring that we're only processing changes once

The last point is necessary because add-ons may register for watching the same patterns as one another or as the Ruby LSP and that results in duplicate changes being sent to the server.

The most optimal solution is #3145, but that is going to require more effort, so let's just fix the issue for now.

### Automated Tests

Added a test and verified manually that the issue is gone.
  • Loading branch information
vinistock committed Feb 6, 2025
1 parent 11d6ef6 commit d48edf2
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 5 deletions.
1 change: 1 addition & 0 deletions lib/ruby_lsp/internal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
require "rbs"
require "fileutils"
require "open3"
require "securerandom"

require "ruby-lsp"
require "ruby_lsp/base_server"
Expand Down
18 changes: 16 additions & 2 deletions lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,19 @@ def run_initialize(message)

# Not every client supports dynamic registration or file watching
if @global_state.client_capabilities.supports_watching_files
send_message(Request.register_watched_files(@current_request_id, "**/*.rb"))
send_message(Request.register_watched_files(
@current_request_id,
Interface::RelativePattern.new(base_uri: @global_state.workspace_uri.to_s, pattern: ".rubocop.yml"),
"**/*.rb",
registration_id: "workspace-watcher",
))

send_message(Request.register_watched_files(
@current_request_id,
Interface::RelativePattern.new(
base_uri: @global_state.workspace_uri.to_s,
pattern: "{.rubocop.yml,.rubocop}",
),
registration_id: "rubocop-watcher",
))
end

Expand Down Expand Up @@ -999,6 +1008,11 @@ def text_document_definition(message)
sig { params(message: T::Hash[Symbol, T.untyped]).void }
def workspace_did_change_watched_files(message)
changes = message.dig(:params, :changes)
# We allow add-ons to register for watching files and we have no restrictions for what they register for. If the
# same pattern is registered more than once, the LSP will receive duplicate change notifications. Receiving them
# is fine, but we shouldn't process the same file changes more than once
changes.uniq!

index = @global_state.index
changes.each do |change|
# File change events include folders, but we're only interested in files
Expand Down
14 changes: 11 additions & 3 deletions lib/ruby_lsp/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -176,19 +176,27 @@ class Request < Message
class << self
extend T::Sig

sig { params(id: Integer, pattern: T.any(Interface::RelativePattern, String), kind: Integer).returns(Request) }
sig do
params(
id: Integer,
pattern: T.any(Interface::RelativePattern, String),
kind: Integer,
registration_id: T.nilable(String),
).returns(Request)
end
def register_watched_files(
id,
pattern,
kind: Constant::WatchKind::CREATE | Constant::WatchKind::CHANGE | Constant::WatchKind::DELETE
kind: Constant::WatchKind::CREATE | Constant::WatchKind::CHANGE | Constant::WatchKind::DELETE,
registration_id: nil
)
new(
id: id,
method: "client/registerCapability",
params: Interface::RegistrationParams.new(
registrations: [
Interface::Registration.new(
id: "workspace/didChangeWatchedFiles",
id: registration_id || SecureRandom.uuid,
method: "workspace/didChangeWatchedFiles",
register_options: Interface::DidChangeWatchedFilesRegistrationOptions.new(
watchers: [
Expand Down
19 changes: 19 additions & 0 deletions test/server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,25 @@ def test_did_change_watched_files_does_not_fail_for_non_existing_files
end
end

def test_did_change_watched_files_processes_unique_change_entries
@server.expects(:handle_rubocop_config_change).once
@server.process_message({
method: "workspace/didChangeWatchedFiles",
params: {
changes: [
{
uri: URI::Generic.from_path(path: File.join(Dir.pwd, ".rubocop.yml")).to_s,
type: RubyLsp::Constant::FileChangeType::CHANGED,
},
{
uri: URI::Generic.from_path(path: File.join(Dir.pwd, ".rubocop.yml")).to_s,
type: RubyLsp::Constant::FileChangeType::CHANGED,
},
],
},
})
end

def test_workspace_addons
create_test_addons
@server.load_addons
Expand Down

0 comments on commit d48edf2

Please sign in to comment.