No more project.razor.bin files (in VS, at least) #10475
Conversation
Since the lifetime of the `RazorProjectInfoDriver` is the same as the language service, we let our language server client create and initialize it during activation. This means that the driver no longer needs to check if `IsLSPEditorAvailable()` since its constructed by the LSP editor's language client.
As part of this change, I've made `RazorProjectService` own the misc files project, rather than `SnapshotResolver`, which fixes a potential issue with initialization order.
…CapabilitiesManager
This is no longer used by Visual Studio and VS Code never used it.
A couple of changes to match the original implementation in ProjectConfigurationFileChangeDetector
There was a problem hiding this comment.
Initialization seems a bit odd to me. The RazorProjectService initialization is tied to the initialization of another service, which is indirectly tied to IOnInitialize, but it seems like if the project server adds itself as a listener in IOnInitialize then the driver could start pushing work in IOnInitialized and nothing would need specific initialization at all. The file watching driver would know that the workspace folder is available, because IOnInitialize is always before IOnInitialized, and only one thing (the driver) would be doing the initial push for the current state of project infos.
...soft.AspNetCore.Razor.LanguageServer/ProjectSystem/FileWatcherBasedRazorProjectInfoDriver.cs
Outdated
Show resolved
Hide resolved
| { | ||
| // If the language server was not created with an IRazorProjectInfoDriver, | ||
| // fall back to a FileWatcher-base driver. | ||
| services.AddSingleton<IRazorProjectInfoDriver, FileWatcherBasedRazorProjectInfoDriver>(); |
There was a problem hiding this comment.
I wonder if the FileWatcherBasedRazorProjectInfoDriver should be constructed by rzls, and passed in to here?
There was a problem hiding this comment.
I had wanted to do that, but it needs the IWorkspaceRootPathProvider, which is tied to the CapabilitiesManager. I'll take another look to see if I can do something better here.
There was a problem hiding this comment.
I took another look. Unfortunately, we just don't know what the root path is when rzls is constructed. Since FileWatcherBasedRazorProjectInfoDriver is dependent on IWorkspaceRootPathProvider, and that's dependent on the CapabilitiesManager, it really needs to be constructed by DI. Maybe we can improve this later, but I think it's probably OK for now.
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Logging/ILoggerFactoryExtensions.cs
Outdated
Show resolved
Hide resolved
|
|
||
| protected override async Task InitializeAsync(CancellationToken cancellationToken) | ||
| { | ||
| var workspaceDirectoryPath = await _workspaceRootPathProvider.GetRootPathAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
I think its a bit odd that we start this up immediately, and then immediately wait for IOnInitialize indirectly through the workspace path provider. I wonder if this self-initialization should/could be replaced with this implementing IOnIntiailized?
There was a problem hiding this comment.
Initially, I tried using IOnInitialized and ran into problems with ordering. Since IOnInitialized offers no guarantees about what order services will be initialized in, it's easy to get into trouble when there are dependencies between services that are all marked for initialization.
There was a problem hiding this comment.
Note that there's no such interface as IOnInitialize. I just want to be sure that we're getting the terminology right. So, I'm stating all of this to make my logic clear:
- There are two LSP messages:
"initialize"and"initialized". The first one happens before the second one. - The first LSP message sent is
"initialize". When received byRazorInitializeEndpoint,CapbilitiesManager.SetInitializeParams(...)is called, making theRootPathandRootUrivalues available. - The
IWorkspaceRootPathProvider.GetRootAsync(...)implementation onCapabilitiesManagerspins until theInitializeParamsbecome available. Once available, it returns the root path value. - The next LSP message should be
"initialized". When received byRazorInitializedEndpoint, it loops through allIOnInitializedLSP services and callsOnInitializedAsync.
In my opinion, it's OK to await IWorkspaceRootPathProvider.GetRootAsync(...) here. This happens before "initialized" and allows FileWatcherBasedRazorProjectInfoDriver to start loading projects early, so it's slightly more likely that the server will be ready to provide project information for the first LSP message that needs it.
There was a problem hiding this comment.
Note that there's no such interface as
IOnInitialize
I think that makes all of my points invalid. I'm not sure where I got the idea from.
There was a problem hiding this comment.
No worries! Another reason I clarified was to make sure that it wasn't just a typo. It could have just been IOnInitialized with the 'd' left off. 😄
| await _projectInfoDriver.WaitForInitializationAsync().ConfigureAwait(false); | ||
|
|
||
| // Register ourselves as a listener to the project driver. | ||
| _projectInfoDriver.AddListener(this); |
There was a problem hiding this comment.
As per a previous comment, the initialization is odd here. This initialization happens immediately, it them initializes the project info driver, which goes through all of the current projects and starts a task for them, then it adds itself as a listener, then this goes through the current projects and updates them. Seems like both this and the driver are trying to inialize a starting set of projects, and if this AddListener call was moved above the WaitForInitializationAsync call then the below loop could be removed?
There was a problem hiding this comment.
In VS, the driver is "hot" and starts populating immediately. If AddListener is moved above WaitForInitializationAsync, I believe there's a potential race condition where incoming project updates could be processed earlier and then rolled back by the loop. For that reason, AbstractRazorProjectInfoDriver.AddListener() throws if called before initialization is finished.
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectSystem/RazorProjectService.cs
Outdated
Show resolved
Hide resolved
Since it hasn't changed, testing this would really just be using the C# extension as is, but specifying the Razor langauge server path to be the |
Thanks for the review! I definitely understand your concern. I'm probably being a bit overprotective around initialization of RazorProjectService to avoid bad start-up state. I'll take another look and make this better. In general, I think I'd like to avoid going through IOnInitialized at all. Unlike the file watcher case, the VS driver should start "hot", which is what I was initially going for. I'm definitely not trying to optimize for the VS Code case, though I'm very nearly inclined to try and implement MemoryMappedFile approach we discussed last week, just to get rid of the file watchers. 😄 |
`RazorProjectService` doesn't need to force itself to be initialized when the language server is initialized. It's registered as an `IRazorStartupService`, so it starts initializing right away. Since all `RazorProjectService` public entry points await initialization, we don't need to force it to finish initializing in `OnInitialized`. This is especially true if the driver itself implicitly waits for `OnInitialized`.
For types that have a `CancellationTokenSource` that is triggered during `Dispose()`, we should check to see if it has already been cancelled to avoid disposing twice. Implicitly, there's a race here if there are multiple threads calling `DIspose()`, but these are all handled by the DI containers we use.
…st that deleted bin file
There was a problem hiding this comment.
Thought about the initialization more, and I think that its not worth blocking on, and I suspect not possible to actually change how I suggested. I got it in my head that there was a way of running services when both initialize and initialized messages were received, but as there isn't, the point is largely moot. After sleeping on it, I'm also not sure how I expect it to work anyway, since the driver would have to be activated on initialized, but its not owned by the language server which recieves that message, nor can it be.
`AbstractRazorProjectInfoDriver`can't call `InitializeAsync(...)` in its constructor because the driver will only be partially constructed. To address that, add a `StartInitialization` method that drivers call from their constructor. This will kick off initialization and set the result of a `TaskCompletionSource` when it finishes.
Instead of spinning, use a TaskCompletionSource to track initialization and an AsyncLazy for the root path.
Two major changes being introduced: dotnet/razor#10480 moves our project change listener to AsyncBatchingWorkQueue to improve stability dotnet/razor#10475 rearchitects our project syncing. Expected to have no impact on VS Code
This is a pretty substantial pull request, so I'm getting it out there for review. I have not fully tested the change with VS Code, but Visual Studio is working ok.
This change completely refreshes all of the machinery around how
RazorProjectInfoinstances are passed from the Razor language client to the Razor language server. This is generally a complicated process with a lot of potential for reliability issues. The language client is expected to serialize aRazorProjectInfoinstance representing a project update to a known location within the project's folder on disk. The language server watches for writes to that file (the dreadedproject.razor.binfile, deserializes it and updates the server's project system.Here's how things work after this change:
IRazorProjectInfoDriverabstraction that is optionally passed intoRazorLanguageServerwhen it is created.IRazorProjectInfoListenerto the driver and is notified of project updates and removes. For an update, the listener is provided aRazorProjectInfoinstance. For a remove, it's just given aProjectKey.RazorProjectServiceprovides theIRazorProjectInfoListenerimplementation. This results in a much simpler system with less machinery, allowing several "file detectors" and "state syncrhronizers" to be deleted.IRazorProjectInfoDriveris provided to the language server performs no serialization/deserialization ofRazorProjectInfoinstances. Instead, the language client and server just shared the sameRazorProjectInfoinstances. I'm expecting this to lead to performance and memory allocation wins.IRazorProjectInfoDriverimplementation is provided. So, the language server falls back to a specialFileWatcherBasedRazorProjectInfoDriverimplementation that watches the workspace folder. TheRazorWorkspaceListenerinExernalAcess.RoslynWorkspacestill serializesRazorProjectInfoinstances for VS Code. So, that should work as it did before, though I still need to test it. (I'd happily accept any help on this.)Significant deletions from the language server in this change:
ProjectConfigurationFileChangeDetectorProjectConfigurationFileChangeEventArgsProjectConfigurationStateSynchronizerMonitorProjectConfigurationFilePathEndpointProjectConfigurationStateManagerProjectInfoEndpointRazorProjectInfoDeserializerSignificant deletions from VS in this change:
ProejctConfigurationFilePathChangedEventsArgsProjectConfigurationFilePathStoreRazorProjectInfoEndpointPublisherRazorProjectInfoFileSerializerRazorProjectInfoPublisherRazor-CI for PR Validation: https://dev.azure.com/dnceng/internal/_build/results?buildId=2473027&view=results