- Description Forgetting to unsubscribe from events is a frequent cause of memory leaks and or hard-to find bugs. Every event that is subscribed to, should be unsubscribed from as well.
- Noncompliant Code Any event declaration that has subscribers (+=) but no unsubscribers (-=)
- Compliant Code Any code where subscriptions and un-subscriptions exist.
- Edge Cases Can’t think of any right now - maybe the community can?
- Type: Bug
Hi @norberth,
Thank you for suggesting this rule. It is always appreciated.
I am not a dotnet expert so I did some research to see what kind of false positives we could have. I found the following articles:
- 5 Techniques to avoid Memory Leaks by Events in C# .NET you should know: It describes a few patterns using Weak References to avoid memory leaks without unsubscribing.
- Are you afraid of event handlers because of C# memory leak? Fear not!: It recommends to not worry about unsubscribing when subscriber and publisher have the same lifetime.
Do you agree with these exceptions?
@Pavel_Mikula what is your opinion?
Hello,
I like the rule idea itself. One leaked event reference can prevent large form and all it’s dependencies from being released.
The Weak Reference aggregator is not concern for this issue, since it doesn’t use the += syntax.
The WeakEventHandler can be handled easily and makes sense as an exception.
Problem with this rule is = operator that subscribes the new event and unsubscribes the previous ones. But that’s rarely used in it’s unsubscribing manner.
Major issue is the “same scope” in WPF/WinForms and similar scenarios. It’s typically NOT using -= to unsubscribe local events. Once control is created, event is often added. And the locality of the scope depends on the control tree that the control is added to. For example:
- I create control. I add it to another control. I add that another control to my form/control => it’s local.
- I create control. I add it to another control. And do nothing => it’s a leak. Or add it to another form => it’s a leak.
- I create control in own tree, but subscribe the event to a method that’s outside of my tree.
We can’t track that.
Another source of FP’s are global events with application-lifetime that you don’t need to unsubscribe. For example all Events of this class https://docs.microsoft.com/en-us/dotnet/api/microsoft.win32.systemevents?view=net-5.0
Or any other global-state related custom event that you need to hold during the whole application lifetime.
Hi Pavel,
I do agree that application lifetime events can be a problem but even with those, the rule might make sense.
To accommodate those specifics I can imagine two things. Either simply unsubscribing in a shutdown method, which has no downside, but would satisfy the rule, or keep a list of “well known” lifetime events and exclude those from the get-go.
In my experience, it’s often user-created events that tend to be forgotten.
Best regards
Norbert
Hi Nicolas,
thanks for some research - both articles are very interesting and definitely shed some light on the problem that I’m trying to address with this new rule.
Article #1 basically says “Unsubscribe” and then talks about “how to do that”
- Paragraph 1 & 2 talk about “regular” unsubscribe mechanism. (Which would satisfy the proposed rule).
- The event aggregator is a event-style notification mechanism without directly using the event syntax (Which would not trigger the rule itself)
- Last, not least, the WeakReference, in my opinion would be an edge case that we could detect simply by the fact that “WeakReference<>” is used.
I’m interpreting this article as a general affirmation of the idea itself 
Article #2 is correct, as far as life-time of windows/controls are concerned. It brings up a valid point about the un-subscription of UI related events though.
Summary
To reduce false-positives we could do the following.
- Exclude events from certain namespaces (System.Windows.Forms, …)
- Only consider events declared by one’s own code-base.
Also, I would like to reiterate that this is not only about memory leaks but about potential bugs in general. Missing an un-subscription (& subsequent re-subscription) might cause certain stretches of code to be executed more than once. This is almost always unintended and hard to see at a first glance.
Let me know what you think.
Best regards
Norbert
I’d say the global event’s are acceptable FPs, or solvable by on-shutdown handlers and few well-known exceptions.
The main problem are those local-scoped events. Last time I met this issue, it was WinForms+regular control event+missing unsubscribe+OutOfMemoryException on the client machine.
I’d prefer FPs over FNs in this case, as manual subscription should be manually unsubscribed. As we are aware of those, we can describe it in the RSPEC.
Thanks for both of your inputs @norberth @Pavel_Mikula.
From what I understand there are two possible rules:
=============
1/ Code smell rule: Manual subscription should be manually unsubscribed.
Forgetting to unsubscribe can create memory leaks now or make code execute multiple times (re-subscription). Even if there is no problem now it can happen later on when the code is refactored.
This rule raises an issue when a class subscribes to an even without explicitly unsubscribing.
2/ Bug rule: Subscribing to events should not cause memory leaks or re-subscriptions
This rule raises an issue when there is a memory leak or re-subscription due to a missing event unsubscription.
=============
In the scope of Rule 1 we wouldn’t have False Positives as the goal is not to fix a bug but to make the code less error-prone and more easy to review. Rule 1 would only have exceptions when it is explicit that you don’t need to unsubscribe (ex: WeakReference is used).
In the scope of Rule 2 we would have to focus only on cases where we detect an actual bug. We would have to add many exceptions and the rule would be complex to implement.
It seems that Rule 1 matches more what you expect @Pavel_Mikula @norberth. Am I right?
If it is the case I can specify such a rule and ask @Pavel_Mikula for a review.
1 Like
Code smell approach LGTM
Agree
Out of curiosity - what will the ETA be approximately? 
We have no ETA, the rule has to be specified first.
Hi @Pavel_Mikula, @norberth,
Does this rule match your expectations: RSPEC-6112? In the end I did not add any exception for the time being because WeakReference and WeakEventHandler patterns seem rare at first sight and they are not trivial to detect. I only mentioned them in the implementation ticket.
There is also the equivalent VB.NET RSPEC.
Hi Nicolas,
the description seems accurate. I’m not sure if I would have chosen an implememtation of IDisposable to showcase the un-subscription but I think it acts as a guideline only.
I’m curious how often that rule will trigger once its available Do you have such stats across your cloud offering?
Best regards
Norbert
Hi @norberth,
I chose this example because it seemed to be an obvious Compliant Solution, but again I am not a dotnet expert. If you see a better example feel free to share it and I’ll include it.
For public statistics I would recommend to look at specific open source projects on SonarCloud. You can select the rule and see the issues it raises. Example for SonarLint.
We review issues raised by each rule on popular open source projects during validation. We use this to add exceptions when necessary. Our goal is to help developers without adding unnecessary work, i.e. no False Positives or at least very rare. This process is done on an internal SonarQube instance.
Cheers,
Nicolas
Hi Nicolas,
Does that rule apply to anonymous delegates (lambda functions)?
I’m asking this because the rule https://rules.sonarsource.com/csharp/RSPEC-3244 checks if an anonymous delegate is being unsubscribed, which does not work, however, if the user does not write the anonymous unsubscribe (he just writes the subscribe), will this rule (RSPEC-6112) be triggered?
Thanks