Gracefully handle span mapping failing #78520
Conversation
ghost
added
Area-IDE
untriaged
| { | ||
| // Span mapping didn't succeed. Razor can log telemetry but this still needs to be handled so return all defaults | ||
| // to indicate mapping didn't succeed. | ||
| return roslynSpans.ToImmutableArray(); |
There was a problem hiding this comment.
ok. so it's ok that you're returning an array that is the same length as 'spans', but is full of 'default' values?
There was a problem hiding this comment.
would it make more sense to just do the same logic as before, just have the caller decide 'the lengths are different, so i'll ignore the result'?
basically, do we need this here? (i don't have a good sense for the best layer for this sort of thing). alternative question, should _razorMappingService.MapSpansAsync fail if it can't return the same number of spans as given?
There was a problem hiding this comment.
(i'm ignorant of this area. just trying to understand it better).
There was a problem hiding this comment.
I'm okay with the caller handling that too. In general we don't want this happening at all and I'm going to add some telemetry on the razor side for this
There was a problem hiding this comment.
think if we're going to handle it, we can handle it here to avoid updating a bunch of different callers (and I don't think the callers would do anything different). But I am also OK with this continuing to throw if we expect to fix it. It isn't taking down the server here, and we are in an error case, so an exception seems appropriate.
There was a problem hiding this comment.
dotnet/vscode-csharp#8271 draft for now because I have to head out but I'll test later tonight that the exception goes to the right place and update the razor code to catch
| { | ||
| // Span mapping didn't succeed. Razor can log telemetry but this still needs to be handled so return all defaults | ||
| // to indicate mapping didn't succeed. | ||
| return roslynSpans.ToImmutableArray(); |
There was a problem hiding this comment.
think if we're going to handle it, we can handle it here to avoid updating a bunch of different callers (and I don't think the callers would do anything different). But I am also OK with this continuing to throw if we expect to fix it. It isn't taking down the server here, and we are in an error case, so an exception seems appropriate.
|
/azp run roslyn-integration-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/backport to release/vscode |
|
Started backporting to release/vscode: https://github.com/dotnet/roslyn/actions/runs/14989957792 |
Backport of #78520 to release/vscode /cc @ryzngard ## Customer Impact ## Regression - [ ] Yes - [ ] No [If yes, specify when the regression was introduced. Provide the PR or commit if known.] ## Testing [How was the fix verified? How was the issue missed previously? What tests were added?] ## Risk [High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]
No description provided.