Remove the cancellation token from the calculation of InProgressState.LazyStaleCompilationWithGeneratedDocuments #75001
Conversation
….LazyStaleCompilationWithGeneratedDocuments This will hopefully address multiple recent feedback tickets around Go To Definition occasionally failing. The umbrella pri 0 feedback ticket is https://developercommunity.visualstudio.com/t/Go-to-definition-doesnt-work/10729503 I was able to reproduce this by following the repro outlined by Garry in the feedback ticket, using the "Code Search" window and doing go to definition multiple times. The issue here is that RegularCompilationTracker.FinalizeCompilationWorkerAsync tries to get the value from the Lazy inProgressState.LazyStaleCompilationWithGeneratedDocuments. This lazy holds onto an action that uses a CancellationToken that has already been cancelled, whereas the CancellationToken used by FinalizeCompilationWorkerAsync has not been cancelled. When the action for the lazy is performed, the corresponding OperationCanceledException ends up escaping out of FinalizeCompilationWorker due to the FatalError.ReportAndPropagateUnlessCanceled exception clause failing due to the cancellation token passed into FinalizeCompilationWorkerAsync not being in a cancelled state. The offending Lazy is constructed by RegularCompilationTracker.WithDoNotCreateCreationPolicy's call to the InProgressState ctor. Instead of passing a closure capturing a cancellation token, I've changed the code to instead only add syntax trees to the compilation that are already realized. I'm not 100% confident that this is sufficient, and if not, another alternative I can think of would be to change this to an AsyncLazy.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
On vacation. But this should almost certainly be either an async lazy, or a CancellableLazy. I won't be able to look for a couple of days. |
|
I didn't remember CancellableLazy, I'll try that instead tomorrow once my machine is in a happier state. In reply to: 2332856779 |
Switched to CancellableLazy as it just fell into place easily. |
| @@ -792,8 +792,7 @@ public ICompilationTracker WithDoNotCreateCreationPolicy(CancellationToken cance | |||
| var alreadyParsedTrees = alreadyParsedTreesBuilder.ToImmutableAndClear(); | |||
| var lazyCompilationWithoutGeneratedDocuments = new Lazy<Compilation>(() => this.CreateEmptyCompilation().AddSyntaxTrees(alreadyParsedTrees)); | |||
|
|
|||
| // Safe cast to appease NRT system. | |||
| var lazyCompilationWithGeneratedDocuments = (Lazy<Compilation?>)lazyCompilationWithoutGeneratedDocuments!; | |||
| var lazyCompilationWithGeneratedDocuments = new CancellableLazy<Compilation?>(cancellationToken => lazyCompilationWithoutGeneratedDocuments.Value); | |||
There was a problem hiding this comment.
On mobile, so likely missing something. But why do we need the lazy above this cancelable lazy?
There was a problem hiding this comment.
These two lazys are arguments 2 & 4 to the InProgressState.ctor call below
There was a problem hiding this comment.
Sounds good! I imagine there might be ways to clean this up, but def out of scope for this pr. Thanks for figuring this out!! We might want to consider backporting.
There was a problem hiding this comment.
Sounds good! I imagine there might be ways to clean this up, but def out of scope for this pr. Thanks for figuring this out!! We might want to consider backporting.
….LazyStaleCompilationWithGeneratedDocuments (dotnet#75001) * Remove strong the cancellation token used in the calculation of InProgressState.LazyStaleCompilationWithGeneratedDocuments This will hopefully address multiple recent feedback tickets around Go To Definition occasionally failing. The umbrella pri 0 feedback ticket is https://developercommunity.visualstudio.com/t/Go-to-definition-doesnt-work/10729503 I was able to reproduce this by following the repro outlined by Garry in the feedback ticket, using the "Code Search" window and doing go to definition multiple times. The issue here is that RegularCompilationTracker.FinalizeCompilationWorkerAsync tries to get the value from the Lazy inProgressState.LazyStaleCompilationWithGeneratedDocuments. This lazy holds onto an action that uses a CancellationToken that has already been cancelled, whereas the CancellationToken used by FinalizeCompilationWorkerAsync has not been cancelled. When the action for the lazy is performed, the corresponding OperationCanceledException ends up escaping out of FinalizeCompilationWorker due to the FatalError.ReportAndPropagateUnlessCanceled exception clause failing due to the cancellation token passed into FinalizeCompilationWorkerAsync not being in a cancelled state. The offending Lazy is constructed by RegularCompilationTracker.WithDoNotCreateCreationPolicy's call to the InProgressState ctor. Instead of passing a closure capturing a cancellation token, I've changed the code to instead use CancellableLazy, which takes in a cancellation token at the time the value is requested.
|
Thank you for finding and fixing the issue, I appreciate the effort! One note of concern though. This issue is present on VS 17.11, as mentioned in the ticket on the Developer Community. This issue, #74621, also seems to indicate that the issue was known to be present on 17.11.0 preview 6 as well. The response on the ticket, however, seems to indicate that this is only fixed in 17.12. I don't know if this is the right place to ask, but I hope it's acceptable since I cannot comment on a close ticket on the Developer Community. I'd kindly like to request that the fix be backported to 17.11. Go to Definition has become such an integrated part of many developers' daily work that this gets very frustrating quickly, and if we don't want to run preview versions of Visual Studio -- which is not uncommon -- we would have to wait months for 17.12 to be released. |
|
This was back ported to 17.11. Thanks |
@CyrusNajmabadi How/when will this fix get pushed? I'm still having this issue on 17.11.3. |
See #75007 |
This will hopefully address multiple recent feedback tickets around Go To Definition occasionally failing. The umbrella pri 0 feedback ticket is https://developercommunity.visualstudio.com/t/Go-to-definition-doesnt-work/10729503
I was able to reproduce this by following the repro outlined by Garry in the feedback ticket, using the "Code Search" window and doing go to definition multiple times.
The issue here is that RegularCompilationTracker.FinalizeCompilationWorkerAsync tries to get the value from the Lazy inProgressState.LazyStaleCompilationWithGeneratedDocuments. This lazy holds onto an action that uses a CancellationToken that has already been cancelled, whereas the CancellationToken used by FinalizeCompilationWorkerAsync has not been cancelled. When the action for the lazy is performed, the corresponding OperationCanceledException ends up escaping out of FinalizeCompilationWorker due to the FatalError.ReportAndPropagateUnlessCanceled exception clause failing due to the cancellation token passed into FinalizeCompilationWorkerAsync not being in a cancelled state.
The offending Lazy is constructed by RegularCompilationTracker.WithDoNotCreateCreationPolicy's call to the InProgressState ctor. Instead of passing a closure capturing a cancellation token, I've changed the code to instead only add syntax trees to the compilation that are already realized. I'm not 100% confident that this is sufficient, and if not, another alternative I can think of would be to change this to an AsyncLazy.