Skip to content
This repository was archived by the owner on Apr 12, 2021. It is now read-only.

Added VS.Initialize and VS.JoinableTaskFactory #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added VS.Initialize and VS.JoinableTaskFactory #24

wants to merge 1 commit into from

Conversation

bluetarpmedia
Copy link
Contributor

VS.Initialize is a new, optional method that extension authors can (preferably should) call during their AsyncPackage's InitializeAsync, and pass in their AsyncPackage's JoinableTaskFactory instance.

By default, the VS.JoinableTaskFactory property returns the ThreadHelper.JoinableTaskFactory instance.

But if the extension calls VS.Initialize then VS.JoinableTaskFactory will subsequently return the extension AsyncPackage's JTF instance.

Classes in the Community.VisualStudio.Toolkit should use VS.JoinableTaskFactory.RunAsync instead of ThreadHelper.JoinableTaskFactory.RunAsync for the reasons outlined in #7

As a result, if an extension uses the Community.VisualStudio.Toolkit and chooses to call VS.Initialize then it will benefit from the task tracking that occurs with their extension package's JTF instance, because all async work started by the Toolkit will use their extension's JTF instance.

…fault, the VS.JoinableTaskFactory property returns the ThreadHelper.JoinableTaskFactory instance. But if an extension author chooses to optionally initialize the VS entry point then VS.JoinableTaskFactory will subsequently return the extension's AsyncPackage JTF instance, which is preferred to use over ThreadHelper.
@madskristensen
Copy link
Owner

Great idea, but a few concerns/ideas.

Having a static reference from the VS class might be problematic when you have more than one extension installed using the save version of the Community.VisualStudio.Toolkit.

@reduckted is working on implementing a TookitPackage base class to use instead of AsyncPackage. It could initialize the toolkit by passing on the JTF behind the scenes, so the user doesn't have to remember to do it.

@bluetarpmedia
Copy link
Contributor Author

@madskristensen @reduckted Sounds great, yes that'll be much better than my approach.

How will methods throughout the Toolkit get access to the package's JTF instance? E.g. TaskExtensions.FireAndForget.

@reduckted
Copy link
Contributor

@bluetarpmedia I don't have any answers to your questions (it sounds like those extension methods are going to be tricky), but I will throw this into the mix regarding unit testing: https://github.com/microsoft/vs-threading/blob/fad0c9a9fe47ae0678c81f6ffd253b7a02d21b93/doc/testing_vs.md

The relevant part:

The recommended pattern for testability is that all your code refer to a YourPackage.JoinableTaskFactory property instead of ThreadHelper directly. The YourPackage.Initialize method should initialize this property to ThreadHelper.JoinableTaskFactory.

It sounds like that's along the lines of what you're trying to achieve, which is awesome. I just don't know how the extension methods that are package-independent would access the JTF.

@reduckted
Copy link
Contributor

Having a static reference from the VS class might be problematic when you have more than one extension installed using the save version of the Community.VisualStudio.Toolkit.

What if you walked up the call stack to find which assembly the JTF was requested from? Something like this:
e5b33a2...reduckted:prototype/use-package-jtf

I don't think the performance impact is anything to worry about, especially since we wouldn't need to get any file info from the stack trace. From what I understand, passing fNeedFileInfo: true to the StackTrace constructor is what would cause poor performance.

@madskristensen
Copy link
Owner

This feels a little bit like a solution looking for a problem to me. I've argued in the past for not doing unit test of code with VS dependencies, but instead argued for cleaner architectural separation and have functional or integration tests for the VS parts only. Mocking out VS dependencies is just a hazzle and I'm not sure you're winning much by doing it. It's the functional tests that really matter for the VS layer IMO. Does the ThreadHelper complicate/block the testability of your extensions today?

For MEF extensions without a package class for example, using the ThreadHelper is the only option. I assume that's why it exists in the first place and is being used by VS itself.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants