With this .NET event, is it OK to pass in this IList instance?

Question

Hi folks,

i've got the following code :-

while (....)
{
    var foo = DoTheFooShakeShakeShake(..);
    foos.Add(foo); // foos is an IList<Foo>, btw and is new'd, above.

    if (foos.Count % 100 == 0)
    {
        var e = new CustomFooEventArgs { UserId = whatever, Foos = foos };
        OnFooPewPew(this, e);
        foos.Clear();
    }
}

// We still might have some foo's left over.. so send em off also.
// Excuse the woeful var names, below.
var e2 = new CustomFooEventArgs { UserId = whatever, Foos = foos };
OnFooPewPew(this, e2);

So, i grab all the foo's for some while/loop condition. Every 100 foo's i then fire an event, which passes the list of foo's off to the subscriber. I then clear this list of foos. Once the loop is finished, i then fire off any remaining foo's to the subscriber.

So - if i fire off an event, which contains the list of foo's ... and then i CLEAR that list .. will that mean that the subscriber can possibly get that list, which is now empty? Should I be passing across a COPY of the list ... and then clearing the original list?

Answer 1

By the time you clear the list, the event handlers will already have been executed (they're called synchronously), so it's not an issue.

However, you should avoid passing the list itself, you should pass a copy. Otherwise the event subscribers can keep a reference to the list and mess with it in unpredictable ways...

Answer 2

Assuming you have control over all the subscribers and are working on one thread only, yes, that is fine. Otherwise, if you do not have control over the subscribers (who knows what they will do?) or you are working with the sent collection using multiple threads, you should consider sending a copy of that collection.

Answer 3

Forget about clearing, the fact that you're mutating the list at all is going to wreak havoc for objects that subscribe to your event. If they persist the list that is returned, you're going to be changing the data anytime you add to the list or clear it.

Not only can you mess the subscriber up, they can mess with you, as they can mutate the list and affect your own process.

If that is not the desired behavior (and I can't think that it is), you're going to want to send not even a copy, but an IEnumerable<Foo> or ReadOnlyCollection<Foo>. Because even if you send a copy, if you have multiple subscribers, they'll all receive the same copy, so their mutations will still wreak havoc for one another.

Answer 4

I would have preferred to do this with comments, but don't have the reputation :-)

The paranoid extension of Anthony's answer, where you don't have control of receivers, is to ensure the list copy you pass out is an ReadOnlyCollection<Foo>(*). If you simply copy your list as another List<Foo> or other mutable collection, even if CustomFooEventArgs defines Foos as a non-mutable interface such as IEnumerable<Foo>, a particularly dastardly receiver could cast Foos as a List<Foo> and mutate it.

As for Thomas' answer, there is nothing from stopping the event receivers spawning an Async task using BeginInvoke, unless I'm missing something.

(*) I'd probably have it defined as IEnumerable<Foo> in CustomFooEventArgs though, the fact a ReadOnlyCollection<Foo> is used is an implementation detail IMHO.