Not all types available through IServiceProvider are available through MEF. This is one of those types. Since you can't make the constructor for VisualStudioDiagnosticListTable asynchronous, there are three primary options here:

1. Keep the constructor here the same as it was before, but ensure SVsErrorList has already been requested. In other words, update LoadComponentsInUIContextAsync to call GetServiceAsync(typeof(SVsErrorList)) before requesting an instance of this MEF type. This will ensure the interface is readily available when you GetService on the UI thread.
2. Switch from simple construction to a construction+async initialization approach. After requesting the instance of VisualStudioDiagnosticListTable, immediately await InitializeAsync, and request the instance of SVsErrorList there.
3. Make SVsErrorList lazily requested by asynchronous methods in the type, so the constructor is just a simple non-blocking constructor.

Option 1 is the easiest, but it's also fragile. I have a feeling there is a good fourth option, where we can use MEF metadata to declare the dependencies and then initialize them.

// placed on the VisualStudioDiagnosticListTable class
[DependsOnService(typeof(SVsErrorList))]

// Pseudo-code for LoadComponentsInUIContextAsync
foreach (var requiredService in GetRequiredServices())
{
  await GetServiceAsync(requiredService);
}

❗️ No async void

This is event handler so I have to use async void

You can make the callback a non-async method which invokes the async method through JoinableTaskFactory, using Run or RunAsync as appropriate.

❗️ Don't call AssertIsForeground in an asynchronous method. Instance, use SwitchToMainThreadAsync to switch to the main thread if needed (the switch is a NOP if the code is already on the main thread).

📝 This appears a few times in this pull request, but I only commented on it here.

❕ Pass cancellationToken

💡 A comment here would be good.

You can make the callback a non-async method which invokes the async method through JoinableTaskFactory, using Run or RunAsync as appropriate.

📝 I suspect the problem is here. Try changing this to use _ = instead of await (fire and forget)

📝 This is another one to call in a fire-and-forget manner.

❓ Did you verify that both of these return non-null?

❗️ Pass cancellationToken

if we switch here, why did the caller also switch?

In this case, the caller does not need to switch because it only invokes an asynchronous method, and does not have any affinity itself.

In general, characteristics of the caller are never a consideration for the switching characteristics of the callee.

📝 Let me know what you think of IPreloadService in sharwell/roslyn@ded603f as a way to avoid hard-coding the services here. I don't consider it a perfect solution, so it may or may not be preferable.

@sharwell It looks great! When can I use it?

are these not cancellable?
no configure-awaiting? can you please put .ConfigureAwait(true) to indicate intent?

no configure-awaiting? can you please put .ConfigureAwait(true) to indicate intent?

This is a JTF aware class, so use of ConfigureAwait is discouraged to improve readability.

This is a JTF aware class

How does one know that? I ask honestly since i'm trying to figure out the how to assess code during reviews. I can accept that JTF stuff odesn't need this. But i'm curious how one tells the difference when looking at something like this. Thanks!

How does one know that?

We don't have a great way to tell at the moment. So far only the package initialization code was migrated to the JTF form at the time we switched to AsyncPackage.

In general, my understanding is the break will occur at the Editor Features layer. Layer there and above can assume JTF is in use, and layers below it cannot.

Ok. And the rule is: in the package code itself it should not configure-await? Does that also apply to any async methods in other services that it calls into?

And, for now, should i just consider this in AsyncPackage, and AbstsractLanguageService and those related classes? or is this just AsyncPackage?

in the package code itself it should not configure-await?

It can, but it's not a necessity and can hurt performance by increasing the number of context switches. This especially applies to the initialization code where things tend to need the main thread.

Does that also apply to any async methods in other services that it calls into?

No, callees can do whatever they want as long as they locally adhere to the threading rules.

And, for now, should i just consider this in AsyncPackage, and AbstsractLanguageService and those related classes? or is this just AsyncPackage?

Everything derived from AsyncPackage, including but not limited to types derived from AbstractPackage.

It can, but it's not a necessity and can hurt performance by increasing the number of context switches.

Note: to be clear, i wasn't asking for ConfigureAwait(false). I was specifically just asking about explicit ConfigureAwaits.

The reason i like explicit (regardless of if its true/false) is that it makes it clear that the author thought about this and asked for the right semantics. So even if .ConfigureAwait(true) is technically unnecessary, it helps make it clear the intent of the code without someone having to go: "was that an accidental or intentional omission?".

AbstractPackage2 switched to the UI thread as teh first thing. But this code did not. intentional?

GetServiceAsync is an asynchronous method. The caller doesn't need to switch threads in advance.

I'm looking for some good, clear, rules about the exact patterns we should have for async. Specifically for JTFish code (since i think i have a good grasp on the roslyn rules for non-jtf code). It's hard to review this stuff since i can't tell what is intentional and part of an accepted pattern, vs waht may be a mistake by omission.

For now, assume ConfigureAwait(false) should be applied in everything below EditorFeatures layer. There and up, it's used except in the following situations:

• A method should not combine ConfigureAwait with one of the JTF switching mechanisms (await SwitchToMainThreadAsync or await TaskScheduler.Default). Use one or the other.
  • If an analyzer requires ConfigureAwait, it is preferable to combine ConfigureAwait(true) everywhere with the use of JTF switching mechanisms than to avoid JTF.
• Async UI code and the package initialization code should use SwitchToMainThreadAsync (if necessary) and omit ConfigureAwait overall. Some legacy code has not transitioned to this pattern yet, which is apparent by the explicit use of ConfigureAwait(true).

Can we tweak this at all:

Async UI code and the package initialization code should use SwitchToMainThreadAsync (if necessary) and omit ConfigureAwait overall

Could we make that:

Async UI code and the package initialization code should use SwitchToMainThreadAsync (if necessary) and use .ConfigureAwait(true) overall

Having the ConfigureAwait be explicit in teh code makes it clear that the behavior is intentional and not he result of accidental omission. It's the same reason we ask that CancellationToken.None be explicitly passed instead of having that happen because of a default-parameter-value. By being explicit, someone looking at the code (1, 2, 5, years down the line) can reasonable assert: yes, the intent is clear here. there wasn't an accidental omission.

This sort of explicitness has been extremely valuable for us in the past. It has helped catch a bunch of issues, and it makes reviewing much simpler since it takes the guesswork out of things. It does make things more verbose. But i think that's a virtue here as being overly clear is better for these subtle areas.

I see vs-threading as the writing on the wall for the eventually elimination of widespread littering of ConfigureAwait. We aren't ready to go down that road yet, but it's the target outcome.

I don't like preemptively changing styles when it's not been guaranteed what the final state will be. In this interim period, for example, it does lead to confusion (i.e. both these threads where i'm still not sure if this code is correct :)).

I'm not trying to hold anything back. I'm literally just trying to figure out what the rules are and how to reason about this code given those rules. Right now it's not at all clear to me (even if i take a stance of: "AbstractPackage is special and goes its own way").

Let's just look at this method on its own. First, i have no idea how to reason about what thread it is actually on. It says LoadComponentsInUIContextAsync. However, there is no assert to actualy verify that it's on the UI context. Then, it performs a bunch of async calls that are themselves not clear to me as to their intent (though i can come aronud around to the idea that if you knwo this is JTF, you can maybe squint and view no-configure-await as being intentional). Then, it runs code that affectively says "if i'm not on the UI thread, bad baddy badness will occur", but i see nothing that ensures that safety.

Overall, i find this pattern highly confusing and very difficult to reason about. Invariants are not made clear, and it seems far too easy to accidentally do the wrong thing.

First of all, the discussion is great. It helps highlight things that aren't obvious.

It says LoadComponentsInUIContextAsync.

I renamed the method to LoadComponentsAsync. It's weird because UIContext here is a Visual Studio API concept independent from the concept of "main thread". Asynchronous methods shouldn't use the method name as an indicator of thread affinity, so we should come up with a new name for this method to avoid confusion.

Then, it performs a bunch of async calls that are themselves not clear to me as to their intent

I am very much interested in your opinion of whether the direction of sharwell/roslyn@ded603f makes things clearer (not considering its current lack of documentation).

Then, it runs code that affectively says "if i'm not on the UI thread, bad baddy badness will occur", but i see nothing that ensures that safety.

This is breaking the rules. Affinity should be explicit, not implicit.

... difficult to reason about ... Invariants are not made clear ...

If we add in the explicit SwitchToMainThreadAsync, does it address the confusion within this context?

If we add in the explicit SwitchToMainThreadAsync, does it address the confusion within this context?

I need to see the final form. It's really hard to say without reviewing the code, along with the rules, and determining if it's clear :)

I am very much interested in your opinion of whether the direction of sharwell/roslyn@ded603f makes things clearer (not considering its current lack of documentation).

Overall, yes :). I prefer declarative dependency declarations. Though i did have some comments on that PR around clarity.

@jinujoseph @dotnet/roslyn-ide I think whether we want .ConfigureAwait(true) to be always added or not is debatable. probably better to be discussed in team meeting.

💡 This SwitchToMainThreadAsync can be removed, since the implementation of LoadComponentsInUIContextAsync will already be performing the switch if necessary.

@JieCarolHu @sharwell
is there reason why we still has this method?

if we are going to keep this for whatever reason, is TaskStatusCenter and ErrorList actually require UI thread?

why not use SAsyncServiceProvider to push initializing some service until it is actually get used?

I guess this will be changed to @sharwell new JTF abstraction we will have in Roslyn side once that is checked in?

Actually, here it should be just using JoinableTaskFactory (inherited from AsyncPackage). The MEF container is not (necessarily) initialized yet so we can't use IThreadingContext.

code above already use ComponentHost so, MEF should be available here.