Add mitigation in 17.7 for cases where local and remote host disagree on source generated files. #69965
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
CyrusNajmabadi
force-pushed
the
sgCrash
branch
from
b198cc1
to
4c56cbe
Compare
CyrusNajmabadi
changed the base branch from
release/dev17.8
to
release/dev17.7
…ument or not Docs REorder mistype
CyrusNajmabadi
force-pushed
the
sgCrash
branch
from
4c56cbe
to
f6d6c20
Compare
CyrusNajmabadi
force-pushed
the
sgCrash
branch
from
f6d6c20
to
0271d21
Compare
CyrusNajmabadi
force-pushed
the
sgCrash
branch
from
0271d21
to
f110af5
Compare
CyrusNajmabadi
requested review from
333fred,
dibarbet and
a team
as code owners
3 tasks
sharwell
reviewed
Sep 18, 2023
| @@ -137,6 +137,9 @@ public void AugmentPeekSession(IPeekSession session, IList<IPeekableItem> peekab | |||
| workspace, document.Id, item.SourceSpan.Start, cancellationToken).ConfigureAwait(false)) | |||
| { | |||
| var text = await document.GetTextAsync(project.Solution, cancellationToken).ConfigureAwait(false); | |||
| if (text is null) | |||
| continue; | |||
There was a problem hiding this comment.
❓ Can we put this in CanNavigateToPositionAsync instead?
sharwell
reviewed
Sep 18, 2023
Comment on lines
56
to
57
| bool throwForMissingSourceGenerated = true, | ||
| CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
💡 Overloads are preferred in order to make cancellationToken required on all paths
|
Closing out. Did not get QB approval. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We're seeing a lot of crashes in the wild where the local VS workspace and the Remote OOP workspace disagree on what trees are present within each other (for example, in #69234).
When looking at these crashes, we commonly see stuff like:
indicating that this is an issue primarily with SG docs. Our theory (which we are still collecting data/dumps on) is that there is either a bug in our workspace in terms of how we generate SG docs, or there is a buggy SG out there which doesn't operate deterministically. In either event, the two hosts could then see a different set of documents, which would then cause these crashes.
To address this, we've made a larger/riskier change in main here #69917 to make it so that the local VS workspace always defers to the remote workspace for SG contents. This should ensure that there is no way for these systems to be out of sync.
However, we do not want to port a large/risky change like that back to 17.7. So, instead we're considering mitigating the issue in 17.7 such that the features that are currently affected make themselves resilient to this scenario. This is not something we want to be in 17.8, so if this is accepted, we will undo this change there and only take the full fix.
When this flows into main, we will want to keep the first commit, which corresponds to #69952.
We will not want to keep the second commit.