Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invert IProjectSnapshotChangeTrigger composition #9950

Closed
Tracked by #9806
DustinCampbell opened this issue Feb 16, 2024 · 0 comments · Fixed by #10063
Closed
Tracked by #9806

Invert IProjectSnapshotChangeTrigger composition #9950

DustinCampbell opened this issue Feb 16, 2024 · 0 comments · Fixed by #10063
Assignees
Milestone

Comments

@DustinCampbell
Copy link
Member

Razor's IProjectSnapshotManager has an [ImportMany] for IEnumerable<IProjectSnapshotChangeTrigger>, which is how varies singleton services are created in Razor tooling. This causes several MEF cycles when tryping to rationalize Razor's various DI containers.

@phil-allen-msft phil-allen-msft added this to the 17.10 P2 milestone Feb 22, 2024
DustinCampbell added a commit that referenced this issue Mar 4, 2024
…10024)

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1947624/
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1978114/
_Might_ fix
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1979185/

Recently, I made a change to
`VisualStudioProjectSnapshotManagerAccessor.Instance` to use
`JTF.Run(...)` as a last resort to ensure that the project snapshot
manager is created on the dispatcher (commit
528f1e9). This is necessary because the
project snapshot manager initializes change triggers when it is created,
and change triggers can do significant work during initialization that
should only run on the dispatcher. However, this has been problematic
because it results in `JTF.Run(...)` occurring on thread pool threads,
which can contribute to thread pool starvation.

To avoid `VisualStudioProjectSnapshotManagerAccessor.Instance` needing
to use `JTF.Run(...)`, I've added an
`IProjectSnapshotManagerAccessor.TryGetInstance(...)` method and fixed
up a few call-sites to use it:

* `CSharpVirtualDocumentFactory` - Accesses the project snapshot manager
to get the current list of projects. So, if the project snapshot manager
hasn't been created yet, we can just assume that there aren't any
projects.
* `FallbackProjectManager` - Access the project snapshot manager to try
and get the loaded project for a particular project key. However, if the
project snapshot manager hasn't been created yet, we can assume that
there isn't a loaded project to retrieve.

Note that these are tactical fixes. Ultimately,
`IProjectSnapshotManagerAccessor` will go away once #9950 is finished.
@phil-allen-msft phil-allen-msft modified the milestones: 17.10 P2, 17.10 P3 Mar 11, 2024
DustinCampbell added a commit to DustinCampbell/razor that referenced this issue Mar 11, 2024
…ger completely

Fixes dotnet#9950

This is a pretty significant change that requires refactoring of how the project snapshot manager is exported within Visual Studio. The IProjectSnapshotManagerAccessor implementations and usages have been completely removed, and IProjectSnapshotChangeTrigger has also been deleted. Now, if a service wants to subscribe to project change events, they can import an IProjectSnapshotManager. And, if that service wants to be loaded when the Razor tooling is boot-strapped, they can export themselves as an IRazorStartupService.
DustinCampbell added a commit to DustinCampbell/razor that referenced this issue Mar 12, 2024
…ger completely

Fixes dotnet#9950

This is a pretty significant change that requires refactoring of how the project snapshot manager is exported within Visual Studio. The IProjectSnapshotManagerAccessor implementations and usages have been completely removed, and IProjectSnapshotChangeTrigger has also been deleted. Now, if a service wants to subscribe to project change events, they can import an IProjectSnapshotManager. And, if that service wants to be loaded when the Razor tooling is boot-strapped, they can export themselves as an IRazorStartupService.
DustinCampbell added a commit to DustinCampbell/razor that referenced this issue Mar 12, 2024
…ger completely

Fixes dotnet#9950

This is a pretty significant change that requires refactoring of how the project snapshot manager is exported within Visual Studio. The IProjectSnapshotManagerAccessor implementations and usages have been completely removed, and IProjectSnapshotChangeTrigger has also been deleted. Now, if a service wants to subscribe to project change events, they can import an IProjectSnapshotManager. And, if that service wants to be loaded when the Razor tooling is boot-strapped, they can export themselves as an IRazorStartupService.
DustinCampbell added a commit to DustinCampbell/razor that referenced this issue Mar 12, 2024
…ger completely

Fixes dotnet#9950

This is a pretty significant change that requires refactoring of how the project snapshot manager is exported within Visual Studio. The IProjectSnapshotManagerAccessor implementations and usages have been completely removed, and IProjectSnapshotChangeTrigger has also been deleted. Now, if a service wants to subscribe to project change events, they can import an IProjectSnapshotManager. And, if that service wants to be loaded when the Razor tooling is boot-strapped, they can export themselves as an IRazorStartupService.
DustinCampbell added a commit that referenced this issue Mar 13, 2024
… the Razor Language Server (#10063)

Fixes #9950
Fixes #9515

I recommend reviewing commit by commit.

The primary goal of this pull request is to change how
`IProjectSnapshotManger` is created and how listeners subscribe to it.
Primarily, `IProjectSnapshotChangeTrigger` and
`IProjectSnapshotManagerAccessor` have been removed. To make sure that
services previously exported as `IProjectSnapshotChangeTriggers` are
still loaded, I've introduced the notion of an `IRazorStartupService`.
This is a service that is guaranteed to be loaded when Razor tooling is
bootstrapped in both the VS and Language Server. Often, these startup
services are responsible for keeping the project snapshot manager
up-to-date and need to be loaded before any project updates have
occurred.

In order to remove `IProjectSnapshotManagerAccessor`, I needed to
re-work several tests to use real Razor tooling objects, rather than
mocks. In particular `RenameEndpointTest` received a lot of attention,
though there's still more work that can be done there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants