1
\$\begingroup\$

A friend of mine asked me a question recently. He was needed to subscribe to an event of some object (window button click) and unsubscribe from it on a first call of a handler. Also he noticed that object is dynamically created and his handler is a delegate.

After some thinking I came up with a solution using closures of an object and a delegate itself:

class Program
{
    class A
    {
        public event EventHandler OnRaise;

        public void RaiseEvent(string text)
        {
            Console.WriteLine("a said: " + text);
            if (OnRaise != null)
                OnRaise(this, null);
        }
    }

    static void Main(string[] args)
    {
        var a = new A();

        EventHandler handler = null;
        handler = (x, y) =>
            {
                Console.WriteLine("handler's working");
                a.OnRaise -= handler;
            };

        a.OnRaise += handler;

        a.RaiseEvent("blah blah ");
        a.RaiseEvent("beep beep ");

        handler = null;
        a = null;

        GC.Collect();

        Console.ReadKey();
    }
}

It does all requested behavior but I'm not sure if it's good and safe from memory-leaks. It would be great if anyone can review the code and help with advice or offer a better way to solve the problem.

\$\endgroup\$
4
  • \$\begingroup\$ @Jesse The question is really very concrete, "does this code leak X" is not subjective; it either does or it doesn't, making it appropriate on SO. Were it just, "make this better" (even if "better were properly defined) then it would be a codereview kind of question. \$\endgroup\$
    – Servy
    Commented Apr 10, 2013 at 20:37
  • \$\begingroup\$ @Servy You might be right; it was just a first impression because of word use, I guess. Though, I think I am personally getting hung up on the question isn't about a specific problem. \$\endgroup\$
    – Jesse
    Commented Apr 10, 2013 at 21:39
  • \$\begingroup\$ @Jesse Sorry for misleading. My bad. It was more about the memory-leak. I just meant if someone would like to say "Oh, God! What a crappy code. Even a babysitter can do it better. Check this out: ..." he is free to post here. Thanks for the advice. \$\endgroup\$
    – michaels
    Commented Apr 10, 2013 at 21:55
  • \$\begingroup\$ @michaels No need to apologize, we were all new somewhere once - it happens. I was just trying to provide some insight into why it might not be appropriate here - it was by no means any type of scolding. =) \$\endgroup\$
    – Jesse
    Commented Apr 10, 2013 at 22:24

1 Answer 1

1
\$\begingroup\$

The code that you have will not result in holding onto a reference to any variables that the anonymous handler closes over once that handler is fired and the handler variable leaves scope or is set to something else (i.e. null), even if the A instance is kept alive.

So in short, it's fine.

Note that this is only ever a problem in the first place if the anonymous even handler closes over a variable that is a) expensive to keep alive, i.e. a very large collection of data and b) has a much shorter lifetime than the object that has the event being subscribed to. If the object holding the event goes out of scope before, or very soon after, whatever is being closed over, then the object is not held in memory any longer than if the event handler is unsubscribed. That specific case isn't actually all that common.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the response. Regarding the b) - I was not told any details of a task. Just "anonymous method event handler of some dynamically created window". So for now it's not possible to say if it cause any lifetime related issues in this case. \$\endgroup\$
    – michaels
    Commented Apr 10, 2013 at 22:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.