Sonar-dotnet: Rule S3168: Broaden detection of event handlers

[antoinebj]

Description

There are several frameworks which do not follow the standard signature for event handlers of what are conceptually events.

Here are 2 examples off the top of my head:

1. The Reactive Extensions' IObservable.Subscribe extension method that takes a delegate
2. Prism's EventAggregator and the region navigation callbacks (OnNavigatedTo, etc)

I believe that the approach of the current implementation of S3168 is far too restrictive and leads very easily to disabling this rule altogether.

May I suggest considering methods whose name starts with 'On' as event handlers, and therefore acceptable for async void?
(Of course it would have to be a bit less naïve, like matching with something along the lines of On[^a-z]\w* to avoid accepting OnboardClient())
That would force the user to reconsider using async void if such a prefix were not suitable for the method.

Repro steps

1. Create a method OnPersonRegistered that handles an event raised by Prism's EventAggregator.
2. Make OnPersonRegistered await an asynchronous method (Task.Yield(), for instance) so that it requires a signature with async void.
3. Run analysis with rule S3168 activated

Expected behavior

The method OnPersonRegistered being the handler of what is conceptually and by name (OnSomething) an event, the async void in its signature is unavoidable and should be acceptable.
Therefore, rule S3168 should not be considered violated.

Actual behavior

The analyzer raises a violation for rule S3168.

Related information

• SonarC# Version 6.8.1.4648
• Visual Studio Version 15.5.4

[Evangelink]

Hi @antoinebj,

Thanks for this suggestion. I haven't been using Rx nor Prism for a while (especially the latter) so I am quite rusty.

Regarding Rx, it seems that using async void with Subscribe could lead to unexpected results (fire and forget) and looking at Stack Overflow threads, Paul Betts is often suggesting another approach.

Regarding Prism it seems like they are not planning to support async-await pattern for Event Aggregator as they say it should be fire-and-forget. It looks like we should do something about it.

As the rule is a bug detection and is part of SonarWay it should try to ensure the fewer False Positive possibles (even if it means missing some cases). I'll discuss the point with the team and get back to you asap.

[Evangelink]

Hi @antoinebj,

I have made some tests with both Prism and Rx and I am not entirely sure what to do. On one hand, yes you don't have the choice and so this issue is a FP but on the other hand I am not sure that the fact you don't have the choice but to use the fire-and-forget mechanism will be clear for everyone and so could lead to unexpected behaviors.

I'll keep you posted next week.

[antoinebj]

Hi @Evangelink

Regarding Rx, what Paul Betts or Stephen Cleary suggest on SO are very valid alternatives and so it was a bad example.

Nevertheless, as you point out, Prism's case still remains, and in fact anything that's truly fire and forget and does not follow the object sender + EventArgs signature will cause false positives. I'm far from being acquainted with every framework in .NET, so I cannot give you another example.

I am not sure that the fact you don't have the choice but to use the fire-and-forget mechanism will be clear for everyone

If Rx's had a Subscribe<T>(this IObservable<T>, Action<object, ValueEventArgs<T>>) extension method, and the developer used that method with the following handler:

async void OnHappened(object sender, ValueEventArgs<Foo> e) {}

Sonar would not complain, and the rookie developer would probably never even look for the valid alternatives.
That means that there is already a possibility of false negatives in some cases.
Is it acceptable to increase that possibility in order to reduce that of false positives? I know that the answer is yes for code smells, but I don't know what your policy is for a rule in the "Bug" category.
Perhaps you could let the user choose, there could be a rule parameter for relaxing the rule to accept methods starting with 'On' and accepting the increased risk of false negatives?

At worst what happens in case of a false negative? The developer uses async void, and finds himself in the same situation as any of the "acceptable" async void. He has to properly deal with it, mostly by trying to avoid leaking unhandled exceptions, and accepting shorter stack traces when debugging.

Even relaxed in the way I suggested, the rule would still remain very useful, because any rookie developer would be bound to violate it at some point, and learn to avoid async void as much as possible, training him to proactively look for alternatives.

[antoinebj]

Any updates on this?

[antoinebj]

In case this can help:
https://blogs.msdn.microsoft.com/lucian/2013/02/17/talk-the-new-async-design-patterns/

Async void is for top-level event-handlers only, _and event-like things_.

[Evangelink]

Hi @antoinebj,

Do you still experience such kind of issue with the newer/latest version of SonarC#?

[antoinebj]

Hi @Evangelink,

I've tested the rule again and its behavior is still the same, which means that I would consider the issue still relevant. AFAIK there has been no change to the implementation of this rule in Sonar-C# since this issue was opened.

In our dev team, we still need to suppress the warning:

1. For any async method that needs to be called from a property setter (due to 2-way binding in WPF) and to avoid CS4014
2. For any async implementation of ICommand.Execute()
3. For pub/sub libraries that do not follow the standard .NET convention

Unless you have better ideas, I still advocate to loosen the rule (or provide a parameter to switch its behavior) so that methods with a certain prefix (e.g. "On", "Handle") or suffix (don't have an example) be considered event handlers for the purpose of this rule. Those prefixes or suffixes could be specified or overriden through the rule parameters.
This would at least provide a solution for point 3, but also point 1 as you could name your method "OnChanged".

To me the important thing is for this rule to train the developer to avoid async void when he can return a Task instead. There are too many areas where activating this rule will generate noise rather than be helpful.

[valhristov]

More non-standard event handler types that are widely used:
TreeView.ItemInvoked and all other events on TreeView.

Reported here

[WarBorg]

also Xamarin.Forms uses unconventional event handler conventions for BindableProperties

https://docs.microsoft.com/en-us/xamarin/xamarin-forms/xaml/bindable-properties#detect-property-changes

can you please provide the status of this issue ? it has been 2 years already

[andrei-epure-sonarsource]

We don't have an ETA for it (although we plan to do some FP cleanup later this year and this may get included in the list of FPs to fix).