Use unique IDs for registering watchers #3144
Conversation
vinistock
added
bugfix
Graphite App
How to use the Graphite Merge QueueAdd the label graphite-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
vinistock
force-pushed
the
02-06-use_unique_ids_for_registering_watchers
branch
from
db19077 to
e2f2a52
Compare
vinistock
force-pushed
the
02-06-use_unique_ids_for_registering_watchers
branch
from
e2f2a52 to
1b4c1af
Compare
Merge activity
|
|
I expect this will be a |
None of the add-ons are currently using that helper method. Even Standard and RuboCop are just building the request by hand. Do you feel like it's too risky to not mark as breaking change? |
|
It probably has minimal impact, but we don't know what private or under-development might be using it. Your call. |
vinistock
force-pushed
the
02-06-use_unique_ids_for_registering_watchers
branch
2 times, most recently
from
10ca2ff to
194f9ce
Compare
|
Alright, I made the API compatible with the old style to avoid having to cut a breaking change just for this. I checked every add-on published to rubygems with the |
|
Nice solution. |
### 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.
graphite-app
bot
force-pushed
the
02-06-use_unique_ids_for_registering_watchers
branch
from
194f9ce to
d48edf2
Compare
graphite-app
bot
deleted the
02-06-use_unique_ids_for_registering_watchers
branch
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
didChangeWatchedFileswere sent to a client that was already stopped, resulting in theNotify file events failedmessage.We should always use unique IDs when registering file watchers.
Implementation
I did 3 small things:
.rubocop, which we watched before but I missed in my refactor PRThe 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.