-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Do not crash server if we fail to determine language for non-mutating request #75509
Conversation
3793cf8
to
a13af4a
Compare
a13af4a
to
ba075b2
Compare
src/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/QueueItem.cs
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs
Outdated
Show resolved
Hide resolved
ex = null; | ||
return language; | ||
} | ||
catch (Exception exception) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to report this somehow? it would be nice if we could push on this to get fixed at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catching naked exception feels... bad.
src/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd like us to be a little more careful/precise in exception handling.
Agreed, this is recoverable. From an impl perspective though, i'd prefer we check explicitly for this, and then fail gracefully vs throwing exceptions and catching all exceptions. |
I can do this to have the langue provider return if it could find it (instead of throwing internally). However I will still need to create an exception to serialize down to the client so it can observe the failure. But I can do that inside the queue handling if you'd prefer. |
Yes please. |
Resolves dotnet/vscode-csharp#7656
Sometimes VSCode will send a request or two with the old URI after it renames a temporary file. When we close the file we lose the languageId that they told us about in didOpen (we drop the file info for it) and the server crashes.
Since this is a recoverable error, we should avoid crashing. Instead, we just fail the request and allow the server to continue running, unless it is a mutating request.