Skip to content

fix: Faulty notifications should not bring down the server#18105

Merged
bors merged 1 commit intorust-lang:masterfrom
Veykril:push-rquxwznuuwpu
Sep 12, 2024
Merged

fix: Faulty notifications should not bring down the server#18105
bors merged 1 commit intorust-lang:masterfrom
Veykril:push-rquxwznuuwpu

Conversation

@Veykril
Copy link
Member

@Veykril Veykril commented Sep 12, 2024

Fixes #18055, if a client sends us an unregistered document path in a did save notification it would force us to exit the thread. That is obviously not great behavior, we should be more fallible here

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2024
@Veykril
Copy link
Member Author

Veykril commented Sep 12, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Sep 12, 2024

📌 Commit 3b8fe6d has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 12, 2024

⌛ Testing commit 3b8fe6d with merge 772acef...

@bors
Copy link
Contributor

bors commented Sep 12, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 772acef to master...

@bors bors merged commit 772acef into rust-lang:master Sep 12, 2024
@Veykril Veykril deleted the push-rquxwznuuwpu branch September 13, 2024 19:02
bors added a commit that referenced this pull request Sep 27, 2024
Include buildfiles in VFS

We subscribe to `textDocument/didSave` for `filesToWatch`, but the VFS doesn't contain those files. Before #18105, this would bring down the server. Now, it's only a benign error logged:
```
ERROR notification handler failed handler=textDocument/didSave error=file not found: /foo/bar/TARGETS
```
It's benign, because we will also receive a `workspace/didChangeWatchedFiles` for the file which will invalidate and load it.

Explicitly include the buildfiles in the VFS to prevent the handler from erroring.
lnicola pushed a commit to lnicola/rust that referenced this pull request Oct 8, 2024
…eykril

Include buildfiles in VFS

We subscribe to `textDocument/didSave` for `filesToWatch`, but the VFS doesn't contain those files. Before rust-lang/rust-analyzer#18105, this would bring down the server. Now, it's only a benign error logged:
```
ERROR notification handler failed handler=textDocument/didSave error=file not found: /foo/bar/TARGETS
```
It's benign, because we will also receive a `workspace/didChangeWatchedFiles` for the file which will invalidate and load it.

Explicitly include the buildfiles in the VFS to prevent the handler from erroring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panics on failed to send a message: SendError

3 participants