No Anonymous Delegate Unsubscribe

ID

csharp.no_anonymous_delegate_unsubscribe

Severity

high

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

code_smell, delegate, event, resource-leak

Description

Reports an unsubscription -= whose right-hand side is an anonymous function: a lambda, or a delegate { } anonymous method. Unsubscribing with a method group — Changed -= OnChanged; — is the correct idiom and is not reported.

Rationale

Every anonymous function expression evaluates to a newly created delegate instance. Removal from an invocation list is by delegate equality, so the instance written at the unsubscription site is never the instance that was added at the subscription site: the removal matches nothing and does nothing at all.

The handler therefore stays attached for the whole life of the publisher, and with it everything the closure captured — usually the subscriber itself. The subscriber cannot be collected while the publisher lives, and each subscribe/unsubscribe cycle adds one more handler that will run again the next time the event is raised. Nothing complains, and the line reads exactly like a working unsubscription.

using System;

public class Monitor
{
    private readonly Sensor sensor;

    public void Watch()
    {
        sensor.Changed += (s, e) => Refresh();
    }

    public void Ignore()
    {
        sensor.Changed -= (s, e) => Refresh();          // FLAW, removes nothing
        sensor.Changed -= delegate { Refresh(); };      // FLAW, a second new instance
    }

    public void Detach()
    {
        sensor.Changed -= OnChanged;                    // OK, the same delegate target
    }

    private void OnChanged(object sender, EventArgs e) { Refresh(); }

    private void Refresh() { }
}

Remediation

Keep the handler in a field or a local of delegate type, subscribe with that value, and unsubscribe with the very same value. When the handler needs no captured state, name it as a method and use the method group on both sides, which makes the pairing visible at a glance:

EventHandler handler = (s, e) => Refresh();
sensor.Changed += handler;
// ...
sensor.Changed -= handler;

If a handler genuinely has to unsubscribe itself the first time it runs, capture it in a local before subscribing so the body has a reference to remove.

References