Remove UI thread use of UIContext #79347
Conversation
| --> | ||
| <PackageVersion Include="Microsoft.VisualStudio.SDK" Version="17.13.40008" /> | ||
| <PackageVersion Include="Microsoft.Internal.VisualStudio.Shell.Framework" Version="17.9.36524" /> | ||
| <PackageVersion Include="Microsoft.Internal.VisualStudio.Shell.Framework" Version="17.12.40391" /> |
There was a problem hiding this comment.
This bump was needed because otherwise use of UIContext in a unit test would throw. This turns out to explain a behavior @jaredpar discovered where our subscription of stuff on a UI thread was throwing exceptions in unit tests, but since it was an async method that was fire-and-forget we never noticed.
| <_Dependency Remove="stdole"/> | ||
| <_Dependency Remove="StreamJsonRpc"/> | ||
| <_Dependency Remove="System.Buffers" /> | ||
| <_Dependency Remove="System.ClientModel"/> |
There was a problem hiding this comment.
The version we're pulling in predates the infamous source generator.
There was a problem hiding this comment.
Why do we need to pull it in at all?
There was a problem hiding this comment.
It's pulling in through reference to Microsoft.Internal.VisualStudio.Shell.Framework. I have no idea why.
And frankly, I'm not sure why we have this mechanism generally in DevDivInsertionFiles.csproj. The intent was so if you pull in a new dependency you don't forget to push it into VS if needed. But I'm not sure we've added a new one to this list in awhile.
|
|
||
| /// <summary> | ||
| /// Only read/written on hte UI thread. | ||
| /// Only read/written on the UI thread. |
| } | ||
| // Make sure we unsubscribe this, or otherwise this will cause a leak in unit tests since the UIContext for SolutionClosing is a static that is shared | ||
| // across all tests. | ||
| _solutionClosingContext?.UIContextChanged -= SolutionClosingContext_UIContextChanged; |
There was a problem hiding this comment.
not sure i entirely unnderstand why this is then ok. but i trust you.
There was a problem hiding this comment.
The whole 'there is something static somewhere that is leaking, but this is actually an instance member that we're hooking up to'.
There was a problem hiding this comment.
Since I have to push to the PR, I'll clarify: we call UIContext.FromGuid() and internally that creates a static dictionary of UIContext GUID to -> UIContext object, so each test that runs will get the same object.
jasonmalinowski
force-pushed
the
remove-ui-thread-use-of-uicontext
branch
2 times, most recently
from
3990bca to
9e71d3c
Compare
jasonmalinowski
force-pushed
the
remove-ui-thread-use-of-uicontext
branch
from
9e71d3c to
6f3d714
Compare
jasonmalinowski
force-pushed
the
remove-ui-thread-use-of-uicontext
branch
2 times, most recently
from
9c146eb to
52138bd
Compare
It seems bumping this to a newer version means we can get rid of the workarounds we have in place when a service is unavailable.
This is free-threaded, no reason to hop to the UI thread anymore.
This can move the creation potentially off of the UI thread.
There was one bug in the old code: there was a null check against bstrFileName, which, if that was indeed null, would have thrown an exception later when that null string was passed to a constructor. I can find no evidence in telemetry or old bugs that indeed that can be null, so I removed the check.
This is a backport of 8b12526 so hopefully we have fewer merge conflicts going forward.
It already comes from MEF, which will already dispose it on shutdown. I verified under a debugger it was being disposed twice.
I'm guessing we had this pattern to ensure it was only done once and in a place that was previously UI-thread affinitized. But that's no longer necessary so it's a lot easier just to move this.
MEF can already give us a Lazy<> we don't need to make our own.
This allows any test to mock out items coming from a broker.
These don't really need meaningful implementations, especially since the default workspace services are already trivial implementations.
jasonmalinowski
force-pushed
the
remove-ui-thread-use-of-uicontext
branch
from
52138bd to
2120fa7
Compare
Right now it appears we have a bug that when that update runs, it updates the Solution.WorkspaceVersion which breaks the ability for TryApplyChanges to work later. CodeModel doesn't deal with that case so leaving this in causes CodeModel tests to get flaky. Furthermore, the WhenActivated() calls can't be unsubscribed, and will leak for later tests, which can cause our tests to OOM in CI.
jasonmalinowski
force-pushed
the
remove-ui-thread-use-of-uicontext
branch
from
2120fa7 to
fbfcad3
Compare
6 participants
Per @davkean this no longer requires the UI thread to subscribe to context change events. This lets us simplify how we subscribe to these to avoid subscribing to them in random places on the UI thread.