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.

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

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

Merged
merged 1 commit into from
Sep 12, 2024

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
11 checks passed
@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