Skip to content

Don't crash server if code lens cache entry does not exist #67610

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 2, 2023
Merged

Don't crash server if code lens cache entry does not exist #67610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
109dfb6
Don't crash server if code lens cache entry does not exist
dibarbet Apr 1, 2023
a5137c2
remove unnecessary using
dibarbet Apr 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/Features/LanguageServer/Protocol/Handler/CodeLens/CodeLensCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ public CodeLensCache() : base(maxCacheSize: 3)
/// Cached data need to resolve a specific code lens item
/// </summary>
/// <param name="CodeLensMembers">the list of nodes and locations for codelens members</param>
/// <param name="TextDocumentIdentifier">the lsp document they came from</param>
/// <param name="SyntaxVersion">the syntax version the codelenses were calculated against (to validate the resolve request)</param>
internal record CodeLensCacheEntry(ImmutableArray<CodeLensMember> CodeLensMembers, TextDocumentIdentifier TextDocumentIdentifier, VersionStamp SyntaxVersion);
internal record CodeLensCacheEntry(ImmutableArray<CodeLensMember> CodeLensMembers, VersionStamp SyntaxVersion);
}
4 changes: 2 additions & 2 deletions src/Features/LanguageServer/Protocol/Handler/CodeLens/CodeLensHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(LSP.CodeLensParams r

// Store the members in the resolve cache so that when we get a resolve request for a particular
// member we can re-use the syntax node and span we already computed here.
var resultId = codeLensCache.UpdateCache(new CodeLensCache.CodeLensCacheEntry(members, request.TextDocument, syntaxVersion));
var resultId = codeLensCache.UpdateCache(new CodeLensCache.CodeLensCacheEntry(members, syntaxVersion));

// TODO - Code lenses need to be refreshed by the server when we detect solution/project wide changes.
// See https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1730462
Expand All @@ -63,7 +63,7 @@ public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(LSP.CodeLensParams r
{
Range = range,
Command = null,
Data = new CodeLensResolveData(resultId, i)
Data = new CodeLensResolveData(resultId, i, request.TextDocument)
};

codeLenses.Add(codeLens);
Expand Down
5 changes: 4 additions & 1 deletion src/Features/LanguageServer/Protocol/Handler/CodeLens/CodeLensResolveData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.VisualStudio.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.CodeLens;

/// <summary>
/// Datatype storing the information needed to resolve a particular code lens item.
/// </summary>
/// <param name="ResultId">the resultId associated with the code lens list created on original request.</param>
/// <param name="ListIndex">the index of the specific code lens item in the original list.</param>
internal sealed record CodeLensResolveData(long ResultId, int ListIndex);
/// <param name="TextDocument">the text document associated with the code lens to resolve.</param>
internal sealed record CodeLensResolveData(long ResultId, int ListIndex, TextDocumentIdentifier TextDocument);
19 changes: 12 additions & 7 deletions src/Features/LanguageServer/Protocol/Handler/CodeLens/CodeLensResolveHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,13 @@ public CodeLensResolveHandler(CodeLensCache codeLensCache)
public bool RequiresLSPSolution => true;

public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(LSP.CodeLens request)
=> GetCacheEntry(request).CacheEntry.TextDocumentIdentifier;
=> GetCodeLensResolveData(request).TextDocument;
Copy link
Member Author

@dibarbet dibarbet Apr 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the key line that changed - instead of reading from the cache entry (which in this error scenario doesn't exist) we read from the resolve data (which comes from the request itself). The request will still throw an error, but it will happen later when an error won't take down the whole queue.


public async Task<LSP.CodeLens> HandleRequestAsync(LSP.CodeLens request, RequestContext context, CancellationToken cancellationToken)
{
var document = context.GetRequiredDocument();
var (cacheEntry, memberToResolve) = GetCacheEntry(request);
var resolveData = GetCodeLensResolveData(request);
var (cacheEntry, memberToResolve) = GetCacheEntry(resolveData);

var currentSyntaxVersion = await document.GetSyntaxVersionAsync(cancellationToken).ConfigureAwait(false);
var cachedSyntaxVersion = cacheEntry.SyntaxVersion;
Expand All @@ -61,7 +62,7 @@ public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(LSP.CodeLens request
CommandIdentifier = ClientReferencesCommand,
Arguments = new object[]
{
cacheEntry.TextDocumentIdentifier.Uri,
resolveData.TextDocument.Uri,
request.Range.Start
}
};
Expand All @@ -71,14 +72,18 @@ public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(LSP.CodeLens request
return request;
}

private (CodeLensCache.CodeLensCacheEntry CacheEntry, CodeLensMember MemberToResolve) GetCacheEntry(LSP.CodeLens request)
private (CodeLensCache.CodeLensCacheEntry CacheEntry, CodeLensMember MemberToResolve) GetCacheEntry(CodeLensResolveData resolveData)
{
var resolveData = (request.Data as JToken)?.ToObject<CodeLensResolveData>();
Contract.ThrowIfNull(resolveData, "Missing data for code lens resolve request");

var cacheEntry = _codeLensCache.GetCachedEntry(resolveData.ResultId);
Contract.ThrowIfNull(cacheEntry, "Missing cache entry for code lens resolve request");
return (cacheEntry, cacheEntry.CodeLensMembers[resolveData.ListIndex]);
}

private static CodeLensResolveData GetCodeLensResolveData(LSP.CodeLens codeLens)
{
var resolveData = (codeLens.Data as JToken)?.ToObject<CodeLensResolveData>();
Contract.ThrowIfNull(resolveData, "Missing data for code lens resolve request");
return resolveData;
}
}

53 changes: 53 additions & 0 deletions src/Features/LanguageServer/ProtocolUnitTests/CodeLens/CSharpCodeLensTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.LanguageServer.Handler.CodeLens;
using Newtonsoft.Json;
using Roslyn.Test.Utilities;
using StreamJsonRpc;
using Xunit;
using Xunit.Abstractions;
using LSP = Microsoft.VisualStudio.LanguageServer.Protocol;
Expand Down Expand Up @@ -190,4 +193,54 @@ public async Task TestRecordDeclarationAsync(bool lspMutatingWorkspace)
await using var testLspServer = await CreateTestLspServerAsync(markup, lspMutatingWorkspace, CapabilitiesWithVSExtensions);
await VerifyCodeLensAsync(testLspServer, expectedNumberOfReferences: 0);
}

[Theory, CombinatorialData]
public async Task TestDoesNotShutdownServerIfCacheEntryMissing(bool mutatingLspWorkspace)
{
var markup =
@"class A
{
void {|codeLens:M|}()
{
}

void UseM()
{
M();
}
}";
await using var testLspServer = await CreateTestLspServerAsync(markup, mutatingLspWorkspace);

var textDocument = CreateTextDocumentIdentifier(testLspServer.GetCurrentSolution().Projects.Single().Documents.Single().GetURI());
var codeLensParams = new LSP.CodeLensParams
{
TextDocument = textDocument
};

var actualCodeLenses = await testLspServer.ExecuteRequestAsync<LSP.CodeLensParams, LSP.CodeLens[]?>(LSP.Methods.TextDocumentCodeLensName, codeLensParams, CancellationToken.None);
var firstCodeLens = actualCodeLenses.First();
var data = JsonConvert.DeserializeObject<CodeLensResolveData>(firstCodeLens.Data!.ToString());
AssertEx.NotNull(data);
var firstResultId = data.ResultId;

// Verify the code lens item is in the cache.
var cache = testLspServer.GetRequiredLspService<CodeLensCache>();
Assert.NotNull(cache.GetCachedEntry(firstResultId));

// Execute a few more requests to ensure the first request is removed from the cache.
await testLspServer.ExecuteRequestAsync<LSP.CodeLensParams, LSP.CodeLens[]?>(LSP.Methods.TextDocumentCodeLensName, codeLensParams, CancellationToken.None);
await testLspServer.ExecuteRequestAsync<LSP.CodeLensParams, LSP.CodeLens[]?>(LSP.Methods.TextDocumentCodeLensName, codeLensParams, CancellationToken.None);
var lastCodeLenses = await testLspServer.ExecuteRequestAsync<LSP.CodeLensParams, LSP.CodeLens[]?>(LSP.Methods.TextDocumentCodeLensName, codeLensParams, CancellationToken.None);
Assert.True(lastCodeLenses.Any());

// Assert that the first result id is no longer in the cache.
Assert.Null(cache.GetCachedEntry(firstResultId));

// Assert that the request throws because the item no longer exists in the cache.
await Assert.ThrowsAsync<RemoteInvocationException>(async () => await testLspServer.ExecuteRequestAsync<LSP.CodeLens, LSP.CodeLens>(LSP.Methods.CodeLensResolveName, firstCodeLens, CancellationToken.None));

// Assert that the server did not shutdown and that we can resolve the latest codelens request we made.
var lastCodeLens = await testLspServer.ExecuteRequestAsync<LSP.CodeLens, LSP.CodeLens>(LSP.Methods.CodeLensResolveName, lastCodeLenses.First(), CancellationToken.None);
Assert.NotNull(lastCodeLens?.Command);
}
}