0

Recently I have come across an increasing number of people who have code similar to the following:

private AsynchronousReader r;

public SynchronousReader()
{
    r = new AsynchronousReader();

    // My practice is to put this here
    // and then never remove it and never add it again
    // thus cleaning up the code and preventing constant add/remove.
    //r.ReadCompleted += this.ReadCompletedCallback;
}

private ReadCompletedCallback()
{
    // Remove the callback to "clean things up"...
    r.ReadCompleted -= this.ReadCompletedCallback;

    // Do other things
}

public Read()
{
    r.ReadCompleted += this.ReadCompletedCallback;

    // This call completes asynchronously and later invokes the above event
    r.ReadAsync();
    r.WaitForCompletion();
}

Folks say that this practice is better than the one I indicated above and have given several reasons specific to Silverlight. They state it prevents memory leaks, threading issues, and even that it is the normal practice.

I have not done much Silverlight, but it seems silly to do this still. Are there any specific reasons one would use this method instead of just rigging up the callback in the constructor once and for the lifetime of the object?

This is as simple as I could make my example. Ignore the fact that it's a sort of wrapper that turns an asynchronous object into a synchronous one. I'm only curious about the way events are added and removed.

Michael J. Gray
  • 9,784
  • 6
  • 38
  • 67

2 Answers2

1

In the case you mention it would make sense to hook it up once, but potentially the objects (parent and/or child) may not get garbage collected as the event handlers still reference them.

According to Marc Gavel here

i.e. if we have:

publisher.SomeEvent += target.SomeHandler;

then "publisher" will keep "target" alive, but "target" will not keep "publisher" alive.

A more important point to bear in mind might be the lifespan of the child object. If it is the same as the parent, then one-off subscription in the constructor makes more sense. If it is dynamic you will likely want to remove the handlers as I have seen them leak (resulting in multiple callbacks).

Note: If the constructor-only method turns out to leak objects, you can always put an unsubscribe in the Dispose() I guess, but I can't say I have ever seen that.

Community
  • 1
  • 1
iCollect.it Ltd
  • 92,391
  • 25
  • 181
  • 202
  • Wouldn't it be easier to put `r.ReadCompleted += this.ReadCompletedCallback;` in the constructor ? – Raphaël Althaus Jun 07 '12 at 15:30
  • he wouldn't subscribe to the event in Read(), normally – Brunner Jun 07 '12 at 15:31
  • Well, I would read that anyway. http://stackoverflow.com/questions/298261/do-event-handlers-stop-garbage-collection-from-occuring – Raphaël Althaus Jun 07 '12 at 15:36
  • @HiTechMagic: the way I see it is that he normally subscribes in the constructor and never unsubscribes. The code that's not commented out is what he found in code written by others – Brunner Jun 07 '12 at 15:36
  • The issue is whether or not it's a best practice to put it in the constructor or to leave it in the method that is called which then calls a separate asynchronous method and removes it after the callback is invoked. One point I just though of is if you do it as implemented here, two async calls racing to finish will fail if one completes before the other and the callback has then been removed in the callback that just completed. The code commented out is the practice I want to use, the as implemented code is the bad/messy stuff in my opinion. – Michael J. Gray Jun 07 '12 at 15:37
  • @Raphaël Althaus: Good link. Have added a quote to my answer. – iCollect.it Ltd Jun 07 '12 at 15:39
  • @HiTechMagic To clarify the specific use here, it's just a view model in Silverlight using the MVVM Light Toolkit. There's an asynchronous set up just like this. The child is exclusively owned by the parent and the child is expected to live only as long as the parent. – Michael J. Gray Jun 07 '12 at 15:45
  • @Michael J. Gray: revised after re-reading. In that instance single hookup in the constructor makes more sense so you are correct in your proposed approach. – iCollect.it Ltd Jun 07 '12 at 15:48
  • @HiTechMagic Alright, that's what I had thought. I wrote a comment about a possible race condition resulting in inconsistencies a bit earlier here. What do you think of that as a position to oppose the add/remove when needed/done practice? I think it applies to both cases, even when the child instance is varying. – Michael J. Gray Jun 07 '12 at 15:50
  • @Michael J. Gray: The other "normal" practice you will often see is a an extra removal of a handler on the line before it is added. None of this is thread safe though, so you could indeed throw away responses, but *last-in-wins* is usually what you want from an event handler. You can either go *constructor only*, or "remove it everywhere else" *just to be certain* :) – iCollect.it Ltd Jun 07 '12 at 15:55
  • If I recall correctly, even event adds and removals are thread safe as they do an atomic swap. However, I don't know what the behavior is if an operation is in progress and you remove the callback. I think it will just drop the result. – Michael J. Gray Jun 07 '12 at 16:08
-1

It sounds like you have two issues:

  1. You're attempting to reuse an object that really should only be used once.
  2. That object needs to get properly cleaned up.

You should really either only use an instance of the SynchronousReader object only once (thus avoiding the two async calls racing with one failing to finish like you mentioned elsewhere) or you should implement IDisposable in order to unsubscribe from the event and prevent the memory leak.

A third solution might be possible: keep the single instance of SynchronousReader, but each call to SynchronousReader.Read would create a new instance of AsynchronousReader (rather than storing it as a private field within the instance). Then you could keep most of the code above which you don't like, but which properly handles event subscriptions.

Mike Post
  • 6,355
  • 3
  • 38
  • 48