Swithc to acquiring the component model on a BG thread.

[CyrusNajmabadi]

Fixes AB#1413657

Seeing what breaks here.

[CyrusNajmabadi]

@jasonmalinowski do you see any issue doing things this way?

[CyrusNajmabadi]

@sharwell is the switch to the the BG necessary/desirable here? or would we just acquire from our async thread, adn allow that impl to be on the BG if it wanted to do that itself?

[jasonmalinowski]

@CyrusNajmabadi Functionally it should be fine. My only "concern" would be whether we're able to instead do the async fetch closer to the actual thing that needs it. The downside generally putting things into package load is you're making everything that uses the package wait until this runs, even if the operation then didn't use it. Package load can be triggered for all sorts of random things, some of which may not use MEF and those are being slowed down. At some point we'll get asked why our package load is slow, we'll see this, and then get to sort out if we can remove this and push the work somewhere closer to its actual need.

Now getting off the soapbox, I admit it's not entirely clear to me what the "ideal" approach would be in this case. The editor factory needs the MEF container to actually create editor types. But we have to call RegisterEditorFactory and pass a concrete IVsEditorFactory instance (we can't pass an async lambda to produce it once it's needed), and the EditorFactory APIs (even IVsEditorFactory4) has no async entrypoints. So I'm happy to sign off on this, but would love it if @olegtk or somebody is able to chime in and explain how we're supposed to do this.

[jasonmalinowski]

Actually there might be one small alternative you could try would be to make that property be an async method and then thread through everything, but I suspect that'll touch a lot more than you want. But it'll make it much clearer what stuff is potentially being slowed down by fetching MEF.

[CyrusNajmabadi]

i will try that and see if it's managable.

[CyrusNajmabadi]

It was definitely not managable. So many sync paths that would run into this.

[CyrusNajmabadi]

@CyrusNajmabadi Functionally it should be fine. My only "concern" would be whether we're able to instead do the async fetch closer to the actual thing that needs it. The downside generally putting things into package load is you're making everything that uses the package wait until this runs, even if the operation then didn't use it.

So here's the thing. InitializeAsync already directly needs the component model later due to:

            var miscellaneousFilesWorkspace = this.ComponentModel.GetService<MiscellaneousFilesWorkspace>();
            RegisterMiscellaneousFilesWorkspaceInformation(miscellaneousFilesWorkspace);

And one of the things we're solving is that the CreateEditorFactories call needs this as well:

        protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress)
        {
            await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true);

            await TaskScheduler.Default;
            _componentModel_doNotAccessDirectly = await this.GetServiceAsync<SComponentModel, IComponentModel>(throwOnFailure: true).ConfigureAwait(false);

            await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

            var shell = (IVsShell7)await GetServiceAsync(typeof(SVsShell)).ConfigureAwait(true);
            var solution = (IVsSolution)await GetServiceAsync(typeof(SVsSolution)).ConfigureAwait(true);
            cancellationToken.ThrowIfCancellationRequested();
            Assumes.Present(shell);
            Assumes.Present(solution);

            foreach (var editorFactory in CreateEditorFactories())

So all we're doing is ensuring we can get this component model in an non-blocking fashion prior to it being needed, vs having to immediately block the UI thread just a few instructions later.

[sharwell]

@sharwell is the switch to the the BG necessary/desirable here? or would we just acquire from our async thread, adn allow that impl to be on the BG if it wanted to do that itself?

The explicit switch to background thread is not desirable. GetServiceAsync will yield on its own if the requested value is not available. The expected pattern would be:

1. Move this line after the explicit switch to main thread
2. Use ConfigureAwait(true)

The end result will match what you see for IVsShell7 and IVsSolution a few lines below.

[CyrusNajmabadi]

will do. on it now.

[jasonmalinowski]

I think this should be fine, ignoring the comments that this isn't "ideal"; but it's not clear to me how the ideal is even achieved in your scenario so this is better than nothing. That said there's some good room for followup here.

[jasonmalinowski]

@CyrusNajmabadi This would also be a good place to run the internal VS integration tests prior to merging, since if this is going to break something, that might find it.

[CyrusNajmabadi]

@CyrusNajmabadi This would also be a good place to run the internal VS integration tests prior to merging, since if this is going to break something, that might find it.

how do i do that? :)