Don't register duplicate CPS subscriptions for same project configura… #1572

mavasani · 2017-02-16T01:03:53Z

…tion.

Customer scenario

VS crashes with ArgumentException on opening dotnet core solutions with projects that have additional files.

Bugs this fixes:

Fixes #1563

Workarounds, if any

None.

Risk

Low risk.
We sometimes ended up with duplicate CPS subscriptions for same configured project, causing us to attempt to add duplicate items to the same workspace project in the langauge service and throwing an ArgumentException. Fix is to keep track of project configurations which have already subscribed.

Performance impact

Low

Is this a regression from a previous update?

Yes. Even though we always registered duplicate subscriptions and I can see the same ArgumentException under the debugger on older builds, the exception was getting swallowed up the stack and not crashing VS. My recent PR to move the Language service to JTF causes this code to now execute in a joinable task, and the exception goes unhandled, crashing VS.

Root cause analysis:

Noted above.

How was the bug found?

Dogfooding.

…tion. We ended up with duplicate subscriptions, causing us to attempt to add duplicate items to the same workspace project in the langauge service and throwing an ArgumentException. Fix is to keep track of the project configurations for which we have registered subscriptions. Fixes dotnet#1563

mavasani · 2017-02-16T03:15:34Z

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

@@ -113,9 +117,6 @@ private Task OnProjectFactoryCompletedAsync()

        private async Task OnProjectChangedCoreAsync(IProjectVersionedValue<IProjectSubscriptionUpdate> e, RuleHandlerType handlerType)
        {
-            // TODO: https://github.com/dotnet/roslyn-project-system/issues/353
-            await _commonServices.ThreadingService.SwitchToUIThread();


I moved this call inside the HandleAsync as we now execute the core handler callback inside joinable tasks, and that needs to force UI thread anyways.

mavasani · 2017-02-16T03:19:52Z

Tagging @dotnet/project-system for review

natidea · 2017-02-16T06:39:15Z

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

            if (previousProjectContext != newProjectContext)
            {
-                // Dispose existing subscriptions.
-                DisposeAndClearSubscriptions();


DisposeAndClearSubscriptions [](start = 16, length = 28)

Why is this dispose no longer needed?

Adding the email conversation with @lifengl for more context.

From: Manish Vasani Sent: Wednesday, February 15, 2017 7:18 PM To: Lifeng Lu <lifengl@microsoft.com>; Srivatsn Narayanan <srivatsn@microsoft.com>; Jeremy Meng <Jeremy.Meng@microsoft.com>; Jacob Viau <javia@microsoft.com>; Viktor Veis <viveis@microsoft.com> Cc: David Kean <David.Kean@microsoft.com> Subject: RE: https://github.com/dotnet/roslyn-project-system/issues/1563 That seems to be indeed the case – I receive one additional update on the disposed subscription, and from then on the updates are only on the newly registered subscription. Thanks for your help Lifeng. From: Lifeng Lu Sent: Wednesday, February 15, 2017 4:25 PM To: Manish Vasani <mavasani@microsoft.com>; Srivatsn Narayanan <srivatsn@microsoft.com>; Jeremy Meng <Jeremy.Meng@microsoft.com>; Jacob Viau <javia@microsoft.com>; Viktor Veis <viveis@microsoft.com> Cc: David Kean <David.Kean@microsoft.com> Subject: RE: https://github.com/dotnet/roslyn-project-system/issues/1563 Do you only receive a few updates after that, or you will receive new updates forever? The problem is that dataflow blocks has an internal queue, so even if you remove an subscription, you will still receive data, which has already been added to the queue. Because your code to handle the dataflow change requires to switch to the UI thread immediately, your handler can be blocked for quite some time, and quite some data will be queued in the action block queue. From: Manish Vasani Sent: Wednesday, February 15, 2017 4:20 PM To: Lifeng Lu <lifengl@microsoft.com>; Srivatsn Narayanan <srivatsn@microsoft.com>; Jeremy Meng <Jeremy.Meng@microsoft.com>; Jacob Viau <javia@microsoft.com>; Viktor Veis <viveis@microsoft.com> Cc: David Kean <David.Kean@microsoft.com> Subject: RE: https://github.com/dotnet/roslyn-project-system/issues/1563 So, my fix is now preventing us from creating duplicate subscriptions for this case. However, I still have an unanswered CPS question: invoking Dispose on the subscription links doesn’t seem to stop the original subscription handler from receiving CPS updates. We always dispose all the existing subscriptions here before adding new subscriptions for new set of configured projects, but as I mentioned earlier we still got updates on the original subscription handler. Am I missing something around how subscription dispose is supposed to work?

To summarize, I was assuming that disposing all existing subscriptions and then registering new ones based on new set of active configured projects would avoid duplicate subscriptions. This is not completely true due to the way dataflow queue works, and this caused duplicate updates on the old and new subscription.

@natidea Could we run into similar issues for NuGet restorer? Due to handle duplicate updates?

In NuGetRestorer we ignore updates that do not contain any changes, here: ProjectRestoreInfoBuilder.cs#L33. Also, NuGet's solution restorer is robust to handle multiple restore calls with identical information. I'm not sure about the way we are disposing -- we use ProjectDataSources.SyncLinkTo to create one subscription which is disposed when target framework changes. We can revisit this when we investigate #1503

Thanks for checking. I think regardless, NuGet restorer should be fine handling duplicate updates as you are just kicking off restore when the update happens, you are not building any state based on design time builds.

srivatsn · 2017-02-16T17:25:14Z

@jinujoseph can you please buddy test this change?

mavasani · 2017-02-16T18:46:03Z

@dotnet/project-system can someone please review the fix? This seems to be causing crash on open solution for large number of customer solutions.

@srivatsn @MattGertz I think we should take this to shiproom today - this is a dogfood blocker.

MattGertz · 2017-02-16T19:03:10Z

Agreed; I've been following the thread.

lifengl · 2017-02-16T23:46:35Z

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

@@ -132,12 +133,10 @@ private Task OnProjectFactoryCompletedAsync()
        {


By moving SwitchToUIThread from OnProjectChangedCoreAsync into SwitchToUIThread, now UpdateProjectContextAndSubscriptionsAsync() is running on a background thread, so you may run into race conditions that multiple calls happen in the same time. It looks like we don't protect the code to be thread-safe, for example the Hash table _projectConfigurationsWithSubscriptions is not protected.

Good point. Let me add switching to UI thread in AddSubscriptions.

Where is it?

https://github.com/dotnet/roslyn-project-system/pull/1572/files#diff-91e4b50ea21b1c470749a30e0c4961e3R264

Sorry, saw that now... I didn't get to the latest change.

lifengl · 2017-02-16T23:52:42Z

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

@@ -340,6 +349,7 @@ private void DisposeAndClearSubscriptions()

            _evaluationSubscriptionLinks.Clear();


We only remove subscriptions when the project is closed. What happens if one configuration is removed? I think the subscription will hold many configuration related objects in the memory. It is not a high priority for this release, but eventually we need address it too.

We also need to think about the fact where user has multi-TFM project, we remove one TF from this list and then add it back. Will everything work fine if we remove the subscription in first step and add it back again later? I'll file a separate issue to track this.

Filed #1583

yes, that is a potential problem... fine to track it separately.

…riptions to avoid race conditions

…tions

lifengl

Looks good to me.

mavasani · 2017-02-17T01:15:00Z

Thanks a lot Lifeng!

dnfclas added the cla-already-signed label Feb 16, 2017

mavasani force-pushed the DuplicateSubscriptions branch from a1c5c0e to da2b955 Compare February 16, 2017 03:10

Fix formatting

0b2bb83

mavasani commented Feb 16, 2017

View reviewed changes

natidea reviewed Feb 16, 2017

View reviewed changes

MattGertz added the Pending shiproom approval label Feb 16, 2017

natidea approved these changes Feb 16, 2017

View reviewed changes

srivatsn added the Approved to merge label Feb 16, 2017

lifengl reviewed Feb 16, 2017

View reviewed changes

mavasani mentioned this pull request Feb 17, 2017

Optimize CPS subscriptions in LangaugeServiceHost #1583

Closed

mavasani added 2 commits February 16, 2017 16:51

Address PR feedback and switch to UI thread for registering new subsc…

cadaf60

…riptions to avoid race conditions

Merge remote-tracking branch 'upstream/master' into DuplicateSubscrip…

e37712b

…tions

lifengl reviewed Feb 17, 2017

View reviewed changes

mavasani merged commit 90f82d6 into dotnet:master Feb 17, 2017

mavasani deleted the DuplicateSubscriptions branch February 17, 2017 01:15

tmeschter mentioned this pull request Sep 11, 2017

Show FrameworkReference version in Solution Explorer #2248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't register duplicate CPS subscriptions for same project configura… #1572

Don't register duplicate CPS subscriptions for same project configura… #1572

mavasani commented Feb 16, 2017 •

edited

Loading

mavasani Feb 16, 2017

mavasani commented Feb 16, 2017

natidea Feb 16, 2017

mavasani Feb 16, 2017

mavasani Feb 16, 2017

natidea Feb 16, 2017

mavasani Feb 16, 2017

srivatsn commented Feb 16, 2017

mavasani commented Feb 16, 2017

MattGertz commented Feb 16, 2017

lifengl Feb 16, 2017

mavasani Feb 16, 2017

lifengl Feb 17, 2017

mavasani Feb 17, 2017

lifengl Feb 17, 2017

lifengl Feb 16, 2017

mavasani Feb 16, 2017

mavasani Feb 17, 2017

lifengl Feb 17, 2017

lifengl left a comment

mavasani commented Feb 17, 2017

		@@ -132,12 +133,10 @@ private Task OnProjectFactoryCompletedAsync()
		{

		@@ -340,6 +349,7 @@ private void DisposeAndClearSubscriptions()

		_evaluationSubscriptionLinks.Clear();

Don't register duplicate CPS subscriptions for same project configura… #1572

Don't register duplicate CPS subscriptions for same project configura… #1572

Conversation

mavasani commented Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

mavasani commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srivatsn commented Feb 16, 2017

mavasani commented Feb 16, 2017

MattGertz commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifengl left a comment

Choose a reason for hiding this comment

mavasani commented Feb 17, 2017

mavasani commented Feb 16, 2017 •

edited

Loading