Free-threaded project system APIs #26931

jasonmalinowski · 2018-05-17T01:39:18Z

This produces a new free-threaded, well factored API for adding projects to the VisualStudioWorkspace. The core type is the VisualStudioProject which is a free-threaded API that you can use to push information over. CSharpProjectShim, VisualBasicProject and CPSProject each have an instance of VisualStudioProject that they push things through.

This also introduces a proper batching API: any consumer of VisualStudioProject may call CreateBatchScope() which means all future operations are put a single batch that will be applied at once in a single workspace change. Multiple calls to CreateBatchScope just stack: it only applies when all scopes are closed. You could think of the behavior as roughly analogous to an SQL transaction, except with no rollback capabilities. A batch is only per-project: a batch started on one project doesn't impact another project. There is no concept of a cross-project batch, short of just starting two independent batches and completing them at the same time (which creates two workspace changes.)

The inheritance model here is now smaller. CSharpProjectShim and VisualBasicProject inherit from AbstractLegacyProject, but that's it. CPSProject now inherits nothing, and AbstractProject is here purely for TypeScript back-compat until they're moved onto the new APIs. The expectation is F# and TypeScript both move to VisualStudioProject, which we make public in some form. Then AbstractProject will go away.

AbstractLegacyProject (which is used for csproj and vbproj) still is tied to the UI thread, but the ties should now be more limited and better understood. The primary tangles right now are having to fetch a few things from IVsHierarchies, namely some project properties and the folder structure. The plan for now is AbstractLegacyProject-derived projects will still be called on the UI thread, and we'll determine a new API to remove those remaining issues for them (or just move them onto VisualStudioProject.)

The threading guarantee given to all consumers of VisualStudioProject is it can be called from any thread, and no synchronous calls to the UI thread may be done at that time. There are a few places where we asynchronously kick off work to inspect the running document table, but those are async and should never block.

There is a lock hierarchy now instituted: each VisualStudioProject has a lock which it takes when any method is called on it, this is to ensure straightforward synchronization of it's data for things like batch start/stopping, what's in the batch, etc. There is also a lock in VisualStudioWorkspaceImpl that is acquired when actually modifying the workspace, which is done through an internal ApplyChangeToWorkspace method so it can be called from VisualStudioProject more easily. It is permissible for a VisualStudioProject to call ApplyChangeToWorkspace to acquire the VisualStudioWorkspaceImpl lock. It is not permissible for a lock holder of VisualStudioWorkspaceImpl's lock to acquire any project lock. To this end, some project-to-project tracking information is held in VisualStudioWorkspaceImpl to avoid mistakes -- although the data is a "per project" concept, putting it in the project would cause people to take a project lock and risk deadlocks.

CyrusNajmabadi · 2018-05-17T01:54:50Z

Any interest in eyes? or would you prefer people wait?

src/VisualStudio/CSharp/Impl/ProjectSystemShim/CSharpProjectShim.OptionsProcessor.cs

CyrusNajmabadi · 2018-05-17T01:57:35Z

src/VisualStudio/CSharp/Test/CSharpVisualStudioTest.csproj

@@ -11,6 +11,7 @@
    <TargetFramework>net461</TargetFramework>
    <RuntimeIdentifier>$(RoslynDesktopRuntimeIdentifier)</RuntimeIdentifier>
    <RoslynProjectType>UnitTest</RoslynProjectType>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>


that's scary :)

Where's your sense of adventure? The unsafer, the better!

Our C# library already had unsafe for some fun marshalling hacks we have to do. One thing I was repeatedly being bit by while refactoring this is a lot of unit tests were calling internal members of AbstractProject directly instead of through the interfaces the project system code actually calls through. Fixing this meant our unit tests now have to do some unsafe code to call into the existing unsafe code. 😄

jasonmalinowski · 2018-05-17T17:31:15Z

@CyrusNajmabadi There's still a few weeks of work planned for this. I created the PR only because there's now enough working again that I suspected there's a chance that integration tests would have passed...

CyrusNajmabadi · 2018-05-17T18:00:03Z

@CyrusNajmabadi There's still a few weeks of work planned for this. I created the PR only because there's now enough working again that I suspected there's a chance that integration tests would have passed...

Sounds good. Let me know if/when you'd like eyes!

jasonmalinowski · 2018-05-17T18:33:58Z

@CyrusNajmabadi Oh, once this is tidied up I'll want a hell of a lot of eyes on it. Right now half your comments would be "why is this important bit commented out right now?" 😄

tmat · 2018-08-21T16:38:13Z

src/VisualStudio/CSharp/Impl/ProjectSystemShim/CSharpProjectShim.OptionsProcessor.cs

+                // Get options from the ruleset file, if any, first. That way project-specific
+                // options can override them.
+                ReportDiagnostic? ruleSetGeneralDiagnosticOption = null;
+                /*


Work in progress?

src/VisualStudio/CSharp/Impl/ProjectSystemShim/CSharpProjectShim.OptionsProcessor.cs

We initialize AssemblyName when we create the project with the project system name (because the workspace requires we have something). But ocne we get the real path, we need to use the real name.

The existing shim constructor we had wasn't being used -- I'm not sure if this is me accidentally refactoring after I had it working, or they changed their code more recently. In either case, it's easy enough to add another compat constructor.

tmat · 2018-10-11T21:14:02Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Legacy/AbstractLegacyProject.cs

+
+            var projectFilePath = hierarchy.GetProjectFilePath();
+
+            if (projectFilePath != null && !File.Exists(projectFilePath))


if (projectFilePath != null && !File.Exists(projectFilePath)) [](start = 12, length = 61)

Assuming this is a workaround for something that will go away one day?

It's the workaround we've had for 7 years, apparently. (It just wasn't very obvious that the workaround indeed existed.) Hopefully AbstractLegacyProject goes away later in the Dev16 wave.

tmat

There's nothing left but shims at this point, so nothing actually requires it. Previously something must have been creating it by accident but we aren't anymore, and F# trying to use it would cause issues.

F# was requesting the ID, which we'd throw on by accident.

jasonmalinowski · 2018-10-16T17:22:03Z

@dotnet-bot retest this please. (This was affected by the Jenkins outage yesterday.)

src/Workspaces/Core/Portable/Workspace/Workspace.cs

We stopped populating this, which breaks various things like Live Unit Testing and some of our persistence stuff.

We were adding with project unique name, but removing with the DisplayName, which could change over time. I've renamed the map to clarify exactly which "name" is being used. Right now I'm being lazy and just using a foreach across the ImmutableDictionary rather than converting it to a bidirectional map. If this ever becomes a perf problem, it's easy to fix.

This meant some of the StubProjects we create for compatibility shims (which derive from AbstractProject) will now have this set. Otherwise we were passing back null, which didn't end well.

The underlying process of removing a document already handled this, this was an unnecessary block. It was already the case that you could call OnProjectRemoved to remove the entire project that had open files, so this didn't really avoid badness in the first place.

I had implemented ProjectTracker.GetProject to always return shim projects for F#, but TypeScript expects they can call that, and then immediately pass that project to RemoveProject. So for TypeScript projects, we'll keep returning the real thing.

I should have overriden this from the start -- the ExplicitBinPath didn't give you a way to specify you really did want it to be null.

A few places we might try to remove something that we shouldn't.

This would have never worked: we would have been closing documents after we had already removed them.

Two bugs being fixed here: 1. When we remove a document in a non-batch scenario, we close any information related to the open document. If we were in a batch scenario, the SetCurrentSolution we do to apply the new Solution snapshot didn't go through that. Now we do. 2. UpdateSolutionForBatch wasn't calling the delegates given; this would have meant that additional files being added in a batch would have been added a regular source files.

…read Since we're checking the RDT asynchronously, we need a way for integration tests to confirm that.

We might disconnect/reconnect on background threads now, so be ready for that.

This test was broken prior to my change: it never waited for CPS to have completed design time builds before it asserted that everything was done. But before, there was no window where the UI thread was idle and the file wasn't associated with Miscellaneous Files. Now that we're more async, I think it is the case where it can be, and that trips things up.

This adds an API for the project system to notify us about files that can generate .cs files in the workspace. The rest of the implementation is still coming but this unblocks the project system side of things.

Pilchie · 2018-10-19T13:34:39Z

GitHub needs reactions on merge events. So happy to see this merged!!!!!!!!!

jasonmalinowski requested a review from a team as a code owner May 17, 2018 01:39

jasonmalinowski added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 17, 2018

jasonmalinowski self-assigned this May 17, 2018

CyrusNajmabadi reviewed May 17, 2018

View reviewed changes

src/VisualStudio/CSharp/Impl/ProjectSystemShim/CSharpProjectShim.OptionsProcessor.cs Show resolved Hide resolved

CyrusNajmabadi reviewed May 17, 2018

View reviewed changes

jcouv added the Area-IDE label May 21, 2018

jasonmalinowski force-pushed the free-threaded-project-system-api branch 2 times, most recently from 0a0f0f4 to 68565ad Compare May 29, 2018 20:31

jasonmalinowski force-pushed the free-threaded-project-system-api branch 3 times, most recently from 685b619 to 8e10b6b Compare June 14, 2018 17:43

jasonmalinowski force-pushed the free-threaded-project-system-api branch from 8e10b6b to 51bd27a Compare August 17, 2018 19:16

jasonmalinowski requested a review from a team as a code owner August 17, 2018 19:16

jasonmalinowski force-pushed the free-threaded-project-system-api branch from 51bd27a to 7c66439 Compare August 17, 2018 20:45

tmat reviewed Aug 21, 2018

View reviewed changes

src/VisualStudio/CSharp/Impl/ProjectSystemShim/CSharpProjectShim.OptionsProcessor.cs Outdated Show resolved Hide resolved

jasonmalinowski force-pushed the free-threaded-project-system-api branch 8 times, most recently from 7742961 to 3c5e96d Compare August 27, 2018 20:40

jasonmalinowski force-pushed the free-threaded-project-system-api branch 2 times, most recently from e8b21f6 to 597cd1b Compare August 30, 2018 23:32

jasonmalinowski added 2 commits October 10, 2018 18:45

Set AssemblyName when we get the real output path

f71c9ea

We initialize AssemblyName when we create the project with the project system name (because the workspace requires we have something). But ocne we get the real path, we need to use the real name.

Add another compat shim for TypeScript

599da64

The existing shim constructor we had wasn't being used -- I'm not sure if this is me accidentally refactoring after I had it working, or they changed their code more recently. In either case, it's easy enough to add another compat constructor.

tmat reviewed Oct 11, 2018

View reviewed changes

tmat approved these changes Oct 11, 2018

View reviewed changes

jasonmalinowski added 3 commits October 11, 2018 15:22

Hook up intermediate output paths for CPS projects

70300b9

Don't require the ProjectTracker to be created on the UI thread

3b48e4c

There's nothing left but shims at this point, so nothing actually requires it. Previously something must have been creating it by accident but we aren't anymore, and F# trying to use it would cause issues.

Make sure the stub project returned from TryGetProjectByBinPath works

49f7123

F# was requesting the ID, which we'd throw on by accident.

CyrusNajmabadi reviewed Oct 16, 2018

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Workspace.cs Show resolved Hide resolved

jasonmalinowski added 5 commits October 16, 2018 16:40

Populate VisualStudioWorkspace's solution file path

248943b

We stopped populating this, which breaks various things like Live Unit Testing and some of our persistence stuff.

Make sure AbstractProject.ProjectFilePath is set

2e8b30d

This meant some of the StubProjects we create for compatibility shims (which derive from AbstractProject) will now have this set. Otherwise we were passing back null, which didn't end well.

jasonmalinowski force-pushed the free-threaded-project-system-api branch from 3eb6a3e to df4adf6 Compare October 16, 2018 23:41

jasonmalinowski and others added 9 commits October 17, 2018 15:12

Let StubProjects correctly return null bin paths

db62d95

I should have overriden this from the start -- the ExplicitBinPath didn't give you a way to specify you really did want it to be null.

Make sure the XamlTextViewCreationListerner's RDT watcher doesn't crash

ff46240

A few places we might try to remove something that we shouldn't.

Remove UpdateSolutionForBatch's documentsToClose

ff1e8f1

This would have never worked: we would have been closing documents after we had already removed them.

Add a Workspace async operation around us queueing stuff to the UI th…

41dda86

…read Since we're checking the RDT asynchronously, we need a way for integration tests to confirm that.

Allow NavigationBarController to handle background thread changes

6c4173f

We might disconnect/reconnect on background threads now, so be ready for that.

Add additional waits for for the Workspace when async things happen

c6a7a72

Add APIs for adding dynamic files to IWorkspaceProjectContext

2e8351a

This adds an API for the project system to notify us about files that can generate .cs files in the workspace. The rest of the implementation is still coming but this unblocks the project system side of things.

jasonmalinowski merged commit c20eae1 into dotnet:dev16.0.x Oct 19, 2018

jasonmalinowski deleted the free-threaded-project-system-api branch October 31, 2018 21:12

jasonmalinowski mentioned this pull request Aug 8, 2019

Clean up dead code (#36138) #37831

Closed

jasonmalinowski mentioned this pull request Jul 28, 2022

Avoid having to take locks repeatedly when cleaning out workspace #62986

Closed

jasonmalinowski mentioned this pull request Feb 21, 2024

Remove code not used in product #72152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Free-threaded project system APIs #26931

Free-threaded project system APIs #26931

jasonmalinowski commented May 17, 2018 •

edited

Loading

CyrusNajmabadi commented May 17, 2018

CyrusNajmabadi May 17, 2018

rynowak May 17, 2018

jasonmalinowski May 17, 2018

jasonmalinowski commented May 17, 2018

CyrusNajmabadi commented May 17, 2018

jasonmalinowski commented May 17, 2018

tmat Aug 21, 2018

tmat Oct 11, 2018

jasonmalinowski Oct 11, 2018

tmat left a comment

jasonmalinowski commented Oct 16, 2018 •

edited

Loading

Pilchie commented Oct 19, 2018 •

edited

Loading


		var projectFilePath = hierarchy.GetProjectFilePath();

		if (projectFilePath != null && !File.Exists(projectFilePath))

Free-threaded project system APIs #26931

Free-threaded project system APIs #26931

Conversation

jasonmalinowski commented May 17, 2018 • edited Loading

CyrusNajmabadi commented May 17, 2018

CyrusNajmabadi May 17, 2018

Choose a reason for hiding this comment

rynowak May 17, 2018

Choose a reason for hiding this comment

jasonmalinowski May 17, 2018

Choose a reason for hiding this comment

jasonmalinowski commented May 17, 2018

CyrusNajmabadi commented May 17, 2018

jasonmalinowski commented May 17, 2018

tmat Aug 21, 2018

Choose a reason for hiding this comment

tmat Oct 11, 2018

Choose a reason for hiding this comment

jasonmalinowski Oct 11, 2018

Choose a reason for hiding this comment

tmat left a comment

Choose a reason for hiding this comment

jasonmalinowski commented Oct 16, 2018 • edited Loading

Pilchie commented Oct 19, 2018 • edited Loading

jasonmalinowski commented May 17, 2018 •

edited

Loading

jasonmalinowski commented Oct 16, 2018 •

edited

Loading

Pilchie commented Oct 19, 2018 •

edited

Loading