Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide API for add-ons to register only unique watchers #3145

Open
vinistock opened this issue Feb 6, 2025 · 0 comments
Open

Provide API for add-ons to register only unique watchers #3145

vinistock opened this issue Feb 6, 2025 · 0 comments
Labels
enhancement New feature or request

Comments

@vinistock
Copy link
Member

We are currently allowing add-ons to register file watchers freely, but I just realized this is not a good approach for a few reasons.

  1. It is better to register for file watching only once with multiple patterns, rather than creating several separate registrations
  2. If registration IDs are not unique, they will cause orphan LSP client objects that continue to receive file change events as we saw in Notify file events failed - Client is not running #3137
  3. If add-ons register for the same pattern, like the RuboCop add-on registering for .rubocop.yml (same as the Ruby LSP), then the server will receive duplicate changes for those patterns

We need to provide a proper API for file watching registration that provides the LSP with the opportunity to:

  1. Register them all at once
  2. Ensure ID uniqueness (it will be a single registration, so that is fixed by default)
  3. Verify that patterns are unique to avoid receiving the duplicate notifications in the first place

I haven't prototyped anything, but I think we may need a breaking change to expose a nicer API to add-ons. Instead of passing the outgoing_queue as a Thread::Queue, it would be better to have our own abstraction so that we can limit what add-ons can pass and ensure that bugs like these cannot happen.
@vinistock vinistock added the enhancement New feature or request label Feb 6, 2025
@vinistock vinistock mentioned this issue Feb 6, 2025
Merged
graphite-app bot pushed a commit that referenced this issue Feb 6, 2025
@vinistock
Use unique IDs for registering watchers (#3144)
d48edf2 
### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@vinistock