fixing up some threading cases #2419

jmarolf · 2017-06-10T01:30:24Z

_tasksService.LoadedProject() assumes that all logic run inside the using is correct and never causes a deadlock. _tasksService.LoadedProjectAsync explicitly uses JTF to ensure no deadlocks occur.

GetService blocks the calling thread where as GetServiceAsync will try several techniques to acquire the service in a non-blocking way before falling back to a blocking approach.

AddSubscriptionsAsync was locking but then calling asynchronus methods and not waiting for them to complete. This could have led to races or interactions with the project after it had closed.

@davkean to double check my understanding of this threading code

Ask mode template forthcoming

jmarolf · 2017-06-10T21:02:22Z

@dotnet/project-system for review

tmeschter · 2017-06-12T18:37:22Z

...Managed.VS/ProjectSystem/VS/Tree/Dependencies/CrossTarget/CrossTargetSubscriptionHostBase.cs

@@ -78,7 +78,7 @@ internal abstract class CrossTargetSubscriptionHostBase : OnceInitializedOnceDis

        protected async Task AddInitialSubscriptionsAsync()
        {
-            using (_tasksService.LoadedProject())


So previously the object returned by LoadedProject would have been disposed at the end of the block. Do we need to do something similar to the object returned by LoadedProjectAsync?

No _tasksService.LoadedProject() blocked until it got a lock, releasing the lock when it was disposed. _tasksService.LoadedProjectAsync asynchronously waits for a lock, executes the lambda, and then released the lock.

In reply to: 121492671 [](ancestors = 121492671)

tmeschter · 2017-06-12T20:14:50Z

...Managed.VS/ProjectSystem/VS/Tree/Dependencies/CrossTarget/CrossTargetSubscriptionHostBase.cs

@@ -249,7 +249,7 @@ await subscriber.Value.InitializeSubscriberAsync(this, _activeConfiguredProjectS

            await _commonServices.ThreadingService.SwitchToUIThread();

-            using (_tasksService.LoadedProject())
+            await _tasksService.LoadedProjectAsync(() =>


ConfigureAwait(true) for this await?

LoadedProjectAsync returns a JoinableTask which always resumes onto the same thread as its caller.

In reply to: 121514747 [](ancestors = 121514747)

davkean · 2017-06-13T03:28:22Z

Can we split this into 3 separate commits?

jmarolf · 2017-06-13T17:46:09Z

@davkean split this into separate commits.

jmarolf · 2017-06-13T17:50:17Z

...Managed.VS/ProjectSystem/VS/Tree/Dependencies/CrossTarget/CrossTargetSubscriptionHostBase.cs

@@ -262,10 +262,12 @@ await subscriber.Value.InitializeSubscriberAsync(this, _activeConfiguredProjectS

                    foreach (var subscriber in Subscribers)
                    {
-                        subscriber.Value.AddSubscriptionsAsync(newProjectContext);
+                        Shell.ThreadHelper.JoinableTaskFactory.Run(()=> subscriber.Value.AddSubscriptionsAsync(newProjectContext));


Its unclear to me what the correct behavior is here. This section of code is in a lock yet previously we would call subscriber.Value.AddSubscriptionsAsync(newProjectContext) and not observe the returned task or wait for it to complete. That seems to imply that we don't care about the lock, since we previously exited the lock statement without any guarantee that the async tasks had finished. Here I am making the assumption that this was an error and we only want to exit the lock once all subscriptions have been added.

davkean · 2017-06-14T00:34:11Z

...Managed.VS/ProjectSystem/VS/Tree/Dependencies/CrossTarget/CrossTargetSubscriptionHostBase.cs

@@ -262,10 +262,12 @@ await subscriber.Value.InitializeSubscriberAsync(this, _activeConfiguredProjectS

                    foreach (var subscriber in Subscribers)
                    {
-                        subscriber.Value.AddSubscriptionsAsync(newProjectContext);
+                        Shell.ThreadHelper.JoinableTaskFactory.Run(() => subscriber.Value.AddSubscriptionsAsync(newProjectContext));


What problem are we solving here? You're adding a synchronous blocking call in a threadpool thread, which is the cause of all these issues here: #2297.

Oh we're not observing it - why isn't this just an await?

you cannot await inside a lock in C# (for obvious reasons).

Its unclear to me what the correct thing to do here is. Previously, we were in a lock but we did not wait for async operations to complete before leaving the lock.

either:

This code does not need to be in the lock, in which case we should move it out of the lock

This code does need to be in the lock, in which case we need to actually wait for the async operations to complete or we risk bugs from threading behavior.

Which is why we're using semaphores elsewhere. Is this actually fixing a known race? if not, I'd leave it until I rewrite this logic of coordination between configured projects. CPS want us to approach it a different way.

Is this actually fixing a known race

I am not aware of one,. Sounds like this is 2 and we should not exit the lock until this subscription is finished.

Which is why we're using semaphores elsewhere

Can you show me where else in the code we do that?

I'd leave it until I rewrite this logic of coordination between configured projects

Fair enough, do we have an issue # for how CPS wants us to do this?

See _gate in LanguageServiceHost.

Looks a lot better, any objections to just doing that?

I have objectiins to doing in 15.3 unless we're solving an actual existing bug. This becomes easy to introduce other issues and other bugs such as deadlocks on shutdown.

ie We've regressed so many times around this code - i'd prefer not to change it unless it's fixing a bug that we're running into right now.

davkean · 2017-06-14T00:35:14Z

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

@@ -266,14 +265,16 @@ private JoinableTask ExecuteWithinLockAsync(Func<Task> task)

                    _projectConfigurationsWithSubscriptions.Add(configuredProject.ProjectConfiguration);
                }
-            }
+
+                return Task.CompletedTask;


~~Why is this needed?~~

Ah, the differ confused me - I see it's for func, not the parent method

davkean

You need to remove the JoinableTaskFactory.Run.

srivatsn · 2017-06-14T04:30:53Z

If this is not addressing any known perf regressions then let's retarget this for 15.6

jmarolf · 2017-06-20T07:45:22Z

@davkean can you comment on whether you want the remaining changes or not?

davkean · 2017-06-20T07:47:24Z

You pulled the semaphore? I'm okay with the other changes.

jmarolf · 2017-06-20T09:08:25Z

@davkean yeah, the semaphore code definitely has problems but lets tackle that in a different PR.

dnfclas added the cla-already-signed label Jun 10, 2017

jmarolf requested a review from davkean June 10, 2017 01:30

tmeschter reviewed Jun 12, 2017

View reviewed changes

jmarolf added 2 commits June 13, 2017 10:41

get services asynchronously

ba84fa3

use LoadedProjectAsync where applicable

a004d10

jmarolf force-pushed the async-useage/use-async-apis-when-possible branch from e2bee26 to dc4e581 Compare June 13, 2017 17:45

jmarolf commented Jun 13, 2017

View reviewed changes

davkean reviewed Jun 14, 2017

View reviewed changes

davkean requested changes Jun 14, 2017

View reviewed changes

srivatsn changed the base branch from master to dev15.6 June 17, 2017 00:35

jmarolf force-pushed the async-useage/use-async-apis-when-possible branch from b6e81da to a004d10 Compare June 19, 2017 22:31

davkean approved these changes Jun 20, 2017

View reviewed changes

jmarolf merged commit 6b982a4 into dotnet:dev15.6 Jun 20, 2017

jmarolf deleted the async-useage/use-async-apis-when-possible branch October 1, 2018 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing up some threading cases #2419

fixing up some threading cases #2419

jmarolf commented Jun 10, 2017

jmarolf commented Jun 10, 2017

tmeschter Jun 12, 2017

jmarolf Jun 12, 2017

tmeschter Jun 12, 2017

jmarolf Jun 12, 2017

davkean commented Jun 13, 2017

jmarolf commented Jun 13, 2017

jmarolf Jun 13, 2017

davkean Jun 14, 2017

davkean Jun 14, 2017

jmarolf Jun 14, 2017

davkean Jun 14, 2017

jmarolf Jun 14, 2017

davkean Jun 14, 2017

jmarolf Jun 14, 2017

davkean Jun 14, 2017

davkean Jun 14, 2017

davkean Jun 14, 2017 •

edited

Loading

davkean Jun 14, 2017

davkean left a comment

srivatsn commented Jun 14, 2017

jmarolf commented Jun 20, 2017

davkean commented Jun 20, 2017

jmarolf commented Jun 20, 2017

fixing up some threading cases #2419

fixing up some threading cases #2419

Conversation

jmarolf commented Jun 10, 2017

jmarolf commented Jun 10, 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

davkean commented Jun 13, 2017

jmarolf commented Jun 13, 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

Choose a reason for hiding this comment

davkean Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean left a comment

Choose a reason for hiding this comment

srivatsn commented Jun 14, 2017

jmarolf commented Jun 20, 2017

davkean commented Jun 20, 2017

jmarolf commented Jun 20, 2017

davkean Jun 14, 2017 •

edited

Loading