Don't use batching work queue in ProjectConfigurationStateSynchronizer
#10308
Conversation
Fixes dotnet#10306 The recent change to use an `AsyncBatchingWorkQueue` in `ProjectConfigurationStateSynchronizer` has resulted in a signficant regression. The syncrhonizer has somewhat subtle semantics that doesn't mesh well with `AsyncBatchingWorkQueue`. As part of rolling this change back, I've commented the code a bit more thoroughly to call out how it works and where the semantics are a bit flaky. Ultimately, I expect we'll be able to get rid of this once we stop writing `project.razor.vs.bin` files to disk.
DustinCampbell
changed the title
ProjectConfigurationStateSynchronizer
...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs
Show resolved
Hide resolved
|
|
||
| namespace Microsoft.AspNetCore.Razor.LanguageServer; | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Love this detailed comment! Is the issue before that async batching could cause problems when doing a lookup for projectkey to file?
There was a problem hiding this comment.
The problem was that the project would get added early enough. Essentially, the async batching queue leveled the playing field so that everything happens in the same queue. However, the project add needs to happen before enqueueing an update. Otherwise, we get into bad behavior where a remove followed by an add (which happens when we delete and write a new file) causes files to be migrated to and from the misc project needlessly. @davidwengier wrote up a good explanation in #10306.
There was a problem hiding this comment.
For the record, I had a branch that kept the async batching queue, and just collapsed a Reset-then-Add to an Update, and it fixed the issue, so we could revisit this in future. At least we better understand the traps to look out for.
Also worth noting that even with the async batching queue, the integration tests pass. I noticed the issue due to some debug asserts in the MoveDocument method on RazorProjectService, and scouring the logs, but that was all caused by things happening between the reset and the add, and after the add the final state of things was all correct.
There was a problem hiding this comment.
Agreed. I actually think things worked with the async batching queue. At least, it never failed for me when loading up OrchardCore. It just did a bunch of unnecessary work that I fully agree that we don't want to do. I'd prefer deleting this type altogether rather than spending too much more time on it.
There was a problem hiding this comment.
I put up #10330 as an alternative to this. Curious for peoples thoughts.
ryzngard
left a comment
ryzngard
left a comment
There was a problem hiding this comment.
💯 love the additional comment context for understanding
|
Closing this first attempt, which has been superseded by three others! 😄 |
…#10335) Fixes #10306 This change takes a different approach than #10308 and #10330 and #10332 because life was meant to be interesting. The original problem we were trying to solve is that when a Reset then Add came in, we would migrate all of the documents out of the project, then migrate them all back in again. The various solutions in the other PRs you can read about at your leisure, this one takes a rather simple approach: 1. Ignore everything but the most recent update for a specific project 2. If we need to update a project, but it doesn't exist, add it In the long run we should remove `IRazorProjectService.AddProjectAsync` entirely, and rename `IRazorProjectService.UpdateProjectAsync` to `ResetProjectAsync`, but doing that now would just conflict with other open PRs.
4 participants
Fixes #10306
The recent change to use an
AsyncBatchingWorkQueueinProjectConfigurationStateSynchronizerhas resulted in a signficant regression. The syncrhonizer has somewhat subtle semantics that doesn't mesh well withAsyncBatchingWorkQueue. As part of rolling this change back, I've commented the code a bit more thoroughly to call out how it works and where the semantics are a bit flaky. Ultimately, I expect we'll be able to get rid of this once we stop writingproject.razor.vs.binfiles to disk.