Expose IVSGitExt as async VS service #1510

jcansdale · 2018-02-27T21:12:05Z

This PR does the following

Initializes PullRequestStatusBarPackage completely on a background thread
Expose IVSGitExt as an async VS service
Expose IVSGitExt as a MEF service
Initialize in background so as not to delay MEF loading
~~Extend IGitHubServiceProvider with GetServiceAsync<T>() and <T, Ret>~~
Use a delegate for GetServiceAsync
- This avoids choosing one of the numerous and incompatible IAsyncServiceProvider implementations
Explicitly construct the correct implementation of IVSGitExt for the running DTE version
- With the bonus of avoiding MEF errors for this service

It takes over from where #1506 left off, doing initialization asynchronously using GetServiceAsync to retrieve IGitExt. This means that we don't need to explicitly switch to the Main thread when retrieving IGitExt.

Lock prevent tasks being executed at the same time but doesn't stop them being started out of order.

PendingTasks is used to execute tasks in order on the thread pool.

This will allow this service to be used by a package that is initializing on a background thread.

…t-pr

jcansdale · 2018-02-27T21:18:23Z

src/GitHub.TeamFoundation.14/Services/VSGitExt.cs

+            PendingTasks = InitializeAsync();
+        }
+
+        async Task InitializeAsync()


We could explicitly call this after constructing the service.

Since IVSGitExt is also consumed as a MEF service, we probably want the VS service to return as soon as possible (since there is no async loading for MEF services). Awaiting InitializeAsync before the service returns is probably a bad idea. Better to continue initializing in the background.

jcansdale · 2018-02-28T09:32:53Z

src/GitHub.VisualStudio/GitHubPackage.cs

    }

+    [PartCreationPolicy(CreationPolicy.Shared)]
+    public class VSGitExtPart


I put this next to the export for IGitHubClient. Maybe we should collect all of the service exports together into a single file?

This shouldn't be here (and GitHubClient shouldn't be either so ignore that example :P). Ideally it should be right next to the implementation, but since we don't want to have multiple exports for both teamfoundation assemblies, and we don't have a separate assembly yet for mef implementations, this should probably go into GitHub.Exports or into the Services folder here in GitHub.VisualStudio.

It should also follow the convention of the other mef dispatchers and be called VSGitExtDispatcher. For it to actually work as a MEF dispatcher, it needs to implement IVSGitExt and have a mef export for that interface.

jcansdale · 2018-02-28T09:39:11Z

src/GitHub.VisualStudio/GitHubPackage.cs

+
+        IVSGitExt CreateVSGitExt(IGitHubServiceProvider sp)
+        {
+            switch (VSVersion.Major)


In the past I've always used DTE.Version to differentiate versions of Visual Studio. This always returns a string with the major version number with a .0 (even for different minor versions). I used VSVersion because it was there, but I think it might hit the file system when reading the file version number.

Keep it consistent with GetService<T> and GetService<T, Ret>. Fix failing VSGitExt unit tests.

Rather than extending IGitHubServiceProvider, pass a delegate for GetServiceAsync.

shana · 2018-02-28T14:44:40Z

src/GitHub.VisualStudio/GitHubPackage.cs

+                    return new Lazy<IVSGitExt>(() => new VSGitExt14(GetServiceAsync)).Value;
+                case "15.0":
+                    return new Lazy<IVSGitExt>(() => new VSGitExt15(GetServiceAsync)).Value;
+                default:


The logic to construct the correct type should be in the VSGitExtDispatcher, so callers don't have to worry about it.
Also, since it needs to call GetServiceAsync on an IAsyncServiceProvider, it should take IAsyncServiceProvider in its constructor, so we know that that's a dependency that this type has. Later, when GitHubServiceProvider supports async calls, we can implement IAsyncServiceProvider on it and pass it in without major changes to the code.

shana · 2018-02-28T15:41:40Z

src/GitHub.VisualStudio/Services/VSGitExtFactory.cs

+            switch (dte.Version)
+            {
+                case "14.0":
+                    return new Lazy<IVSGitExt>(() => new VSGitExt14(sp.GetServiceAsync)).Value;


Why not just pass the IAsyncServiceProvider into VSGitExt14/15 instead?
Also, any specific reason why you need a Lazy here? You're evaluating it immediately after creating, which feels like it defeats the purpose of the Lazy in the first place.

Why not just pass the IAsyncServiceProvider into VSGitExt14/15 instead?

The multiple implementations of IAsyncServiceProvider wreck havoc. The GitHub.TeamFoundation assemblies pick up more dependencies and I end up having to use extern alias. Passing GetServiceAsync as a delegate ends up being much simpler.

You're evaluating it immediately after creating, which feels like it defeats the purpose of the Lazy in the first place.

I've added some comments and change it to use Func<IVSGitExt> instead (using Lazy was a bit of a hack). By referencing VSGitExt14 and VSGitExt15 inside a lambda expression we can avoid loading the incompatible version into VS (e.g. we mustn't load VSGitExt15 into VS 2015).

shana · 2018-02-28T15:42:35Z

src/GitHub.VisualStudio/Services/VSGitExtFactory.cs

+
+        public async static Task<IVSGitExt> Create(IAsyncServiceProvider sp)
+        {
+            var dte = await sp.GetServiceAsync(typeof(DTE)) as DTE;


ApplicationInfo.GetVersion will give you the running VS version as a proper type, so you don't have to grab a separate service for that here

Cache the call to GetService<IVSGitExt>(). Use Func<IVSGitExt> rather than Lazy<IVSGitExt> (which was a hack).

- Use `UIContext.WhenActivated` instead of UIContext.UIContextChanged event - Don't use async InitializeAsync now we're initializing on background thread - No more need to use ContinueWith

Simplify the VSGitExt service

jcansdale added 8 commits February 23, 2018 10:52

Ensure consistent task ordering using ContinueWith

323f330

Lock prevent tasks being executed at the same time but doesn't stop them being started out of order.

Rename InitializeTask to PendingTasks

5200e72

PendingTasks is used to execute tasks in order on the thread pool.

Do initialization asynchronously on Main thread

68d098a

This will allow this service to be used by a package that is initializing on a background thread.

Add comment about using from background thread

dcd806f

Factor out GetServiceAsync

2a3cef5

Make grammarful English

16aabcd

Merge branch 'fixes/1493-using-ContinueWith' into feature/show-curren…

cf2bc71

…t-pr

Expose IVSGitExt as async VS service

f23fd24

jcansdale commented Feb 27, 2018

View reviewed changes

jcansdale commented Feb 28, 2018

View reviewed changes

jcansdale changed the title ~~Expose IVSGitExt as async VS service~~ [wip] Expose IVSGitExt as async VS service Feb 28, 2018

jcansdale added 2 commits February 28, 2018 10:59

Add generic GetServiceAsync to IGitHubServiceProvider

921f8c5

Keep it consistent with GetService<T> and GetService<T, Ret>. Fix failing VSGitExt unit tests.

Use DTE.Version to choose IVSGitExt implementaiton

ace5498

jcansdale mentioned this pull request Feb 28, 2018

Allow VSGitExt to be constructed and subscribed to from background thread #1506

Closed

Add some comments

738b046

jcansdale force-pushed the refactor/convert-VSGitExt-to-service branch from 81cc078 to 738b046 Compare February 28, 2018 13:03

jcansdale requested a review from shana February 28, 2018 13:05

jcansdale changed the title ~~[wip] Expose IVSGitExt as async VS service~~ Expose IVSGitExt as async VS service Feb 28, 2018

Use a delegate for GetServiceAsync

f3865b1

Rather than extending IGitHubServiceProvider, pass a delegate for GetServiceAsync.

shana reviewed Feb 28, 2018

View reviewed changes

jcansdale added 4 commits February 28, 2018 15:01

Move VSGitExtPart into VSGitExtDispatcher

cc6a112

Move CreateVSGitExt into VSGitExtFactory

e517687

Merge VSGitExtDispatcher into VSGitExtFactory

b98cc52

Move all construction logic into VSGitExtFactory.Create

719e624

shana reviewed Feb 28, 2018

View reviewed changes

jcansdale added 2 commits February 28, 2018 16:27

Add comments to factory method

af9f364

Cache the call to GetService<IVSGitExt>(). Use Func<IVSGitExt> rather than Lazy<IVSGitExt> (which was a hack).

Use ApplicationInfo.GetHostVersionInfo not DTE.Version

fc06da7

jcansdale force-pushed the refactor/convert-VSGitExt-to-service branch from c78162b to fc06da7 Compare February 28, 2018 18:22

jcansdale mentioned this pull request Mar 1, 2018

GitSccProvider is overly active #1511

Closed

Simplify the VSGitExt service

ee9ab4a

- Use `UIContext.WhenActivated` instead of UIContext.UIContextChanged event - Don't use async InitializeAsync now we're initializing on background thread - No more need to use ContinueWith

jcansdale mentioned this pull request Mar 2, 2018

[Feature] Show current PR on status bar #1396

Merged

9 tasks

Merge pull request #1513 from github/refactor/simplify-VSGitExt

0eadf70

Simplify the VSGitExt service

jcansdale merged commit 32d2805 into feature/show-current-pr Mar 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Expose IVSGitExt as async VS service #1510

Expose IVSGitExt as async VS service #1510

Uh oh!

jcansdale commented Feb 27, 2018 •

edited

Loading

Uh oh!

jcansdale Feb 27, 2018

Uh oh!

jcansdale Feb 28, 2018

Uh oh!

jcansdale Feb 28, 2018

Uh oh!

shana Feb 28, 2018

Uh oh!

jcansdale Feb 28, 2018

Uh oh!

shana Feb 28, 2018

Uh oh!

shana Feb 28, 2018

Uh oh!

jcansdale Feb 28, 2018

Uh oh!

shana Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Expose IVSGitExt as async VS service #1510

Expose IVSGitExt as async VS service #1510

Uh oh!

Conversation

jcansdale commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jcansdale commented Feb 27, 2018 •

edited

Loading