Add thread affinity labeling attributes

[AArnott]

Problem statement

Our newly data-driven VSTHRD010 rule still requires that when any individual type becomes free-threaded, that it must opt out of the namespace rule via an out of band data file. It also doesn't lend easily to affinity flags that vary depending on the version of VS being targeted.

By improving thread affinity labeling of types in the VS SDK, we can increase the confidence that VS and VS extension developers have in themselves when moving code to background threads, increase the likelihood that they do so, with fewer bugs.

Proposed solution

Let's define custom attribute(s) in Microsoft.VisualStudio.Threading.dll that other libraries can use to self-identify as thread-affinitized or free-threaded. When attributes are present, these attributes take precedence over the AdditionalFiles that otherwise help VSTHRD010 understand whether a type or member has thread-affinity. This carries benefits including:

1. It's more clear to someone during a code review
2. Can be changed at any time to match code changes and is more likely to be kept current.
3. Can vary between versions of a consuming library without worrying about partitioning the AdditionalFiles data based on version of the library.

Specifically, here is the public API we would add:

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface | AttributeTargets.Event | AttributeTargets.Property | AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Assembly)]
public class InvokeOnAttribute : Attribute
{
    public InvokeOnAttribute(ThreadAffinity threadingRequirement);

    public ThreadAffinity ThreadingRequirement { get; }
}

public enum ThreadAffinity
{
    MainThread,
    AnyThread,
}

We allow the attribute to be applied to any members that can be invoked (methods, properties, events) of course, since these carry code that may be thread-affinitized. But we also allow the attribute to be applied to types and assemblies as a way of setting the default thread affinity when no more specific attributes applied to the member in question.

When an individual member has no attribute, our VSTHRD010 analyzer will search for one following this order, stopped at the first match:

1. On base members that this one overrides (implicitly, since this attribute is inherited)
2. On the matching member of each interface that this member implements, when applicable
3. On each interface that defines the member being implemented, when applicable
4. On the assembly that defines the interface being implemented by the member
5. On the declaring type
6. On the base type, or the assembly that defines it (recursively for all base types).
7. The containing type (recursively)

When in the above list we reference an interface being implemented, when more than one interface is implemented by a given member, our search order is alphabetical on the full type name.

❓ Should the above list be searched be satisfied based on data from our AdditionalFiles, or strictly only on the use of an attribute? Should attributes always take precedence over AdditionalFiles, or only when certain locality requirements are met?

The VSTHRD010 analyzer would not care whether the attribute is defined by the Microsoft.VisualStudio.Threading.dll library or another one. The attribute may be internal with respect to another library too. This way a library can declare its own thread affinity requirements without taking an assembly reference dependency on the vs-threading library if that isn't desirable for some reason.

Describe alternatives you've considered

We could continue investing in the data file, developing MSBuild targets that can detect the version of VS SDK being targeted and supply different AdditionalFiles items based on that.

A downside to using attributes is if a type or member is mislabeled, fixing it requires an in-band code change and re-shipping the SDK for that library. If all such data is out of band (shipping in the vssdk-analyzers package, for example), fixing an error is a much simpler matter (although it requires that everyone update their package references either way).

[AArnott]

@BertanAygun @lifengl @ikeras I'd appreciate your review of this proposal.

[BertanAygun]

I like the idea of this information living with the code. For most usage cases I assume the attributes would be on service interface definitions mostly I am guessing and having those should help to show that info in docs as well. We would still need the additional files support though due to PIAs.

As to the open question, I would prefer attributes taking precendence all the time if there is conflicting information. If no attributes are present, the data from additional files should be used.

[sharwell]

I think the attributes should have precedence. It would allow corrections for things like this:

public void NotAffinitized() {
  if (IsOnMainThread()) {
    // This incorrectly marks the enclosing method as affinitized, which then propagates
    SomeAffinitizedMethod();
  } else {
    SomeNonAffinitizedMethod();
  }
}

[AArnott]

@sharwell: Can you elaborate? I don't think calling ThreadHelper.CheckAccess() causes a method to be marked as requiring the UI thread. It certainly isn't supposed to, AFAIK.

@BertanAygun: service interface definitions aren't the types that services are cast to, so they'd never be checked for attributes. The attributes have to be defined on types or members that actually get invoked (or cast to, if they are COM types). Keep in mind too, that #263 led to VSTHRD010 no longer considering a cast to be thread affinitized unless the type was a COM imported interface.

[ikeras]

I also like the idea of attribute annotations, as keeping them up to date is much more likely. In terms of precedence though, I would have thought that an external confg file would take precedence. That way if a type or method is incorrectly marked, it's possible for the consumer to override it via the config file.

A few questions:

1. Your list of order precedence doesn't start with the member itself, which I would have expected (vs. it's base)?
2. Should constructors be a target?
3. Do you think there may be value in UI thread vs. Main thread vs. Any thread?
4. What about tlbexp generated assemblies? Note: one thing we did for Debugger annotation attributes (like DebuggerDisplay) was allow them to be put into any assembly, but target types in other assemblies via a Target named param which took a Type.

[sharwell]

@AArnott This method is getting incorrectly treated as requiring the UI thread: https://github.com/gitextensions/NBug/blob/e6d751dc614f408d05d288688de5b808ce4a1eee/NBug/Core/UI/UISelector.cs#L26

[AArnott]

@ikeras asked:

Your list of order precedence doesn't start with the member itself, which I would have expected (vs. it's base)?

The list is preceded by this qualification, which I think brings its behavior in line with your expectations:

When an individual member has no attribute...

@ikeras asked:

Should constructors be a target?

Yes.

Do you think there may be value in UI thread vs. Main thread vs. Any thread?

If there comes such a time we need to distinguish these, we're hosed anyway. At that point, we have distinct JoinableTaskContext instances that have different significant only discernible at runtime. Static analysis cannot longer tell us whether someone's call to JoinableTaskFactory.SwitchToMainThreadAsync took us to the "main thread" vs. the "UI thread". This is one of my primary concerns with splitting the main thread in two. So I'm naming the enum above based on "main thread" vs. "any" because that's the semantics that JTF already has today.

What about tlbexp generated assemblies?

Does anyone actually use tlbexp? If so, that's exciting since I'd like to see more COM interfaces defined in C# than I thought we had. Using Target sounds like a substitute for directly applied attributes, and we already have AdditionalFiles for that, which you seem to make the case earlier for that it should be considered an easier thing to maintain. What would the added benefit be to attributes with Target properties?

[AArnott]

@sharwell: transitive propagation of UI thread requirements only happens when an explicit "throw if not on UI thread" method is invoked within that method. If we otherwise detect what we believe to be thread affinity in a method, it's kept local and you only get the warning locally within that method. I assume the method you link to is the latter (merely a local detection) in which case, I'd have to ask what API it considered thread affinitized and is that not correct? Do you supply AdditionalFiles within your compilation to influence that? We should probably move this discussion to gitter or another issue though, since it's not related to attributes.

[ikeras]

@AArnott I'm fine w/ the additional files model, since it's what the rest of the analyzers use anyway (to annotate types that you don't have access to the source for). I mentioned the Debugger attribute approach only because it has the nice attribute that if you misspell a type, or stop referencing it, it catches at build time vs. an additional file.

I mentioned constructors because it was missing in the AttributeTargets list.

[AArnott]

I mentioned constructors because it was missing in the AttributeTargets list.

Yes, I saw that... just forgot to fix it till now. Thanks.

[lifengl]

I like the idea. For the enum value, Adrian happened to mention to me a 2nd party API, which cannot be called in the UI thread. That sounds somewhat strange for a public API, but internally, we do have methods, which are not expected to be used in the UI thread. (Often it might work but could lead into performance problems.) Or, the method works in both threads, but repeatedly using it in one of them might lead to performance problems. If we can describe the thread preference as well, we can do more code analysis later.

Sent from my phone

On May 8, 2018, at 10:58 AM, Andrew Arnott <[email protected]mailto:[email protected]> wrote:

I mentioned constructors because it was missing in the AttributeTargets list.

Yes, I saw that... just forgot to fix it till now. Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Fvs-threading%2Fissues%2F272%23issuecomment-387488997&data=02%7C01%7C%7C8a279f6d184a4f2bd3d408d5b50d4f05%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636613991111773852&sdata=SUa2M%2FyC0q5RlFvmxd7xycywa1tPSV4sXE%2FeVG647c4%3D&reserved=0, or mute the threadhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FALGWwqoT17JURceDdxyK5_V5OrD944G8ks5twdzGgaJpZM4T2vOc&data=02%7C01%7C%7C8a279f6d184a4f2bd3d408d5b50d4f05%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636613991111773852&sdata=RduZ%2BQ1fvb5im7cQeDjhj90sIYw9POiB0Zr2VLDznSM%3D&reserved=0.

[AArnott]

Thanks for reviewing, @lifengl. Must these methods be synchronous such that the caller has to be aware of this? Or can they be async and switch on their own?

[mgoertz-msft]

Some food for thought...

If we are concerned about analyzer performance we could use this attribute to generate the additional file at build time.

And for cases that cannot use an attribute (because they don't want to or cannot take dependency on MS.VS.Th for example) perhaps an alternative could be to invent a new Doc Comment tag to declare thread affinity.

[AArnott]

We should add an analyzer that would notice when ThreadHelper.ThrowIfNotOnUIThread() is called in a method that is not attributed and report a diagnostic. A code fix should be offered to automatically annotate the method.

[BertanAygun]

I am still wondering about enforcing this on COM interfaces especially for 2019 and earlier interop libraries as I don't believe we can add attributes to those ones so for those we may have to keep the AdditionalFiles approach as the last option in ordering. Otherwise we will likely have a many warnings that we may not be able to fix.

[AArnott]

Yes, we would still support the AdditionalFiles which would be used where attributes are absent. But FWIW I think adding attributes to the older interops gets easier in Dev17 because of the merged interops work providing C# for all the interops that we can actually annotate and document.