Do not parse URIs during LSP serialization/deserialization #76691
Conversation
ghost
added
Area-IDE
untriaged
dibarbet
force-pushed
the
uri_crash
branch
9 times, most recently
from
a6bf829 to
e0e73bb
Compare
| /// TODO: document. | ||
| /// Converts the LSP spec URI string into our custom wrapper for URI strings. | ||
| /// We do not convert directly to <see cref="System.Uri"/> as it is unable to handle | ||
| /// certain valid RFC spec URIs. We do not want serialization / deserialization to fail if we cannot parse the URI. |
There was a problem hiding this comment.
consider linking to the runtime issue.
| // Valid URI, but System.Uri cannot parse it. | ||
| [InlineData(true, "perforce://@=1454483/some/file/here/source.cs")] | ||
| [InlineData(false, "perforce://@=1454483/some/file/here/source.cs")] | ||
| public async Task TestOpenDocumentWithInvalidUri(bool mutatingLspWorkspace, string uriString) |
There was a problem hiding this comment.
new tests for these URIs
| // If either of the URIs cannot be parsed, we'll compare the original URI strings. | ||
| if (otherUri.ParsedUri is null || this.ParsedUri is null) | ||
| { | ||
| return this.UriString == otherUri.UriString; |
There was a problem hiding this comment.
this must be false, right? because we did this exact check above, and would have returned 'true' if it succeeded. so why not return false here?
There was a problem hiding this comment.
yup, that should definitely return false. good catch
| /// In order to gracefully handle these issues, we defer the parsing of the URI until someone | ||
| /// actually asks for it (and can handle the failure). | ||
| /// </remarks> | ||
| internal sealed class DocumentUri : IEquatable<DocumentUri> |
There was a problem hiding this comment.
the main new wrapper type
| /// </summary> | ||
| internal class DocumentUriConverter : JsonConverter<Uri> | ||
| internal class DocumentUriConverter : JsonConverter<DocumentUri> |
There was a problem hiding this comment.
new serializer for new wrapper type
| private class LspUriComparer : IEqualityComparer<Uri> | ||
| { | ||
| public static readonly LspUriComparer Instance = new(); | ||
| public bool Equals(Uri? x, Uri? y) |
| @@ -112,7 +62,7 @@ public int GetHashCode(Uri obj) | |||
| /// the URI. | |||
| /// <para/> Access to this is guaranteed to be serial by the <see cref="RequestExecutionQueue{RequestContextType}"/> | |||
| /// </summary> | |||
| private ImmutableDictionary<Uri, (SourceText Text, string LanguageId)> _trackedDocuments = ImmutableDictionary<Uri, (SourceText, string)>.Empty.WithComparers(LspUriComparer.Instance); | |||
| private ImmutableDictionary<DocumentUri, (SourceText Text, string LanguageId)> _trackedDocuments = ImmutableDictionary<DocumentUri, (SourceText SourceText, string LanguageId)>.Empty; | |||
There was a problem hiding this comment.
rest of this file is interesting - contains changes to attempt to parse and handle issues
|
As for the pause here - I need to update XAML with these changes, but have been working on infra. Will get back to this shortly |
This is ready for re-review. The change is 90% the same, except that I made it backwards compatible by not deleting the original URI properties on the protocol types. Instead they are obsoleted and I added new |
|
validation was clean - https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/634260 |
Resolves dotnet/vscode-csharp#7638
Resolves microsoft/vscode-dotnettools#1932
Related to dotnet/runtime#64707
Problem
System.Uriwill throw attempting to parse URIs that havesub-delimsin the host name. This is because it does additional host name validation beyond what is defined in the URI RFC spec. See above linked issues for specific examples.When we get passed these URIs from VSCode, the server crashes because we cannot successfully deserialize the LSP URI string into a
System.Uri. We are unable to easily recover from these as it happens before we get to the queue processing.Solution
The approach I took in this PR is to stop parsing URIs during LSP message deserialization and remove usages of
System.Urifrom LSP directly. Instead, I created a wrapper typeDocumentUriwhich initially stores just the string representation of the URI (exactly how LSP defines URIs). TheSystem.Urican be optionally retrieved from this (which will parse it). Only places which need to extract information from the URI should retrieve theSystem.Uriand must handle failures.This allows URI parsing to be delayed until much later (for example looking at the scheme or file path to find a matching document in the workspace). While we still cannot parse the URI, we can handle the error and provide a misc document instead of crashing the server. These aren't normal files anyway (they don't conform to the
file:///<path>format) and would go to misc regardless of if we succeed in parsing it or not.If at some point the runtime adds a new parsing mode, or we switch to our own URI parser (similar to O#), this still provides value. Deserialization of the URI string and parsing the URI string are two separate logical operations and should not be tied together. Additionally, any runtime improvements here would only be available in .NET 10.
Key areas to look at
DocumentUriDocumentUriConverterLspWorkspaceManagerProtocolConversionsRazor PR - https://github.com/dotnet/razor/pull/11390/files