C# INotifyPropertyChanged handler

Question

I am starting to read about MVVM and one pattern I see a lot is:

public event PropertyChangedEventHandler PropertyChanged;
//.....

PropertyChangedEventHandler handler = this.PropertyChanged;
if (handler != null)
{
    var e = new PropertyChangedEventArgs(propertyName);
    handler(this, e);
}

Why to bother to declare this handler variable? It just looks to me as complicating code unnecessarily, but I can see this even at Microsoft's own tutorials, why not to just use it as:

if (this.PropertyChanged != null)
{
    var e = new PropertyChangedEventArgs(propertyName);
    this.PropertyChanged(this, e);
}

BradleyDotNET · Accepted Answer · 2015-01-06T19:28:41.193

9

Storing off the PropertyChanged event is for thread-safety. In fact, you should technically be doing this with all your events.

The assignment creates a copy of the event and its handlers (not a reference, which would be useless), which means you avoid the scenario where the event handler is set to null right after it passes the null check. This avoids a potential race condition that would throw a NullReferenceException.

In reality, the UI isn't setting that property to null very often, if at all. To be safe however, and use good practice, you should assign the handler off.

edited Jan 06 '15 at 19:28

answered Jan 06 '15 at 19:10

BradleyDotNET

60,462
10
96
117

Oh Interesting to know! So if the property is null the copies made by handler will call some disposed objects right? Can't this be an issue as well? – Michel Feinstein Jan 06 '15 at 19:17
Sorry, what do you mean by "so the samples don't include it"? I can see this code in almost all the sample codes I can find online – Michel Feinstein Jan 06 '15 at 19:18
@mFeinstein To your first question, there are many cases where the event handler could be set to null but the old objects are not disposed, so thats not *necessarily* an issue, though it *could* be one. To the second, I was referring to the Microsoft tutorials. – BradleyDotNET Jan 06 '15 at 19:19
And I guess there isn't a way to avoid calling that old dead object right? This just makes it safer, but not actually safe, am I wrong? – Michel Feinstein Jan 06 '15 at 19:24
Here is the Microsoft tutorial I saw, and there is the same code... http://msdn.microsoft.com/en-us/library/ms743695%28v=vs.110%29.aspx – Michel Feinstein Jan 06 '15 at 19:25
@mFeinstein No, there is no way to trap that as far as I know. Of course, you could always try/catch the exception, but in years of WPF development, I have *never* had that occur, so I wouldn't worry about it too much. – BradleyDotNET Jan 06 '15 at 19:26
@mFeinstein Huh, I guess they updated their tutorial. Last time I looked I don't remember seeing it. MS got their act together apparently :) – BradleyDotNET Jan 06 '15 at 19:29
Bah. In ctor, `PropertyChanged += (o,e) => {};` and be done. Wastes practically nothing, and you never have to worry about it being null. – Jan 06 '15 at 19:44
I thought about this Will, but it kinda sounded as bad practice to me, I might be wrong but I thought the event handler should be allowed to become null sometime, maybe some other logic can use this to find out about some program state, like "if the handler is null, so this means I can do that thing", but I am not sure – Michel Feinstein Jan 06 '15 at 19:54
@mFeinstein Especially with INPC, you couldn't rely on that, because the handler would so rarely be null. The real bad practice with that is the extra invocation (which is, as the other answer notes, debated). – BradleyDotNET Jan 06 '15 at 19:55
@Will Couldn't you still assign `null` to the event? A quick test shows the assignment is valid. – BradleyDotNET Jan 06 '15 at 19:58

score 6 · Answer 2 · edited May 23 '17 at 11:43

I just wanted to add a couple of points to BradleyDotNET's answer.

In his book CLR via C#, Jeffrey Richter points out that the pattern is still not guaranteed to be thread-safe, because the compiler could optimize away the local variable (although the current version of the compiler does not make this optimization). He recommends using Volatile.Read to be technically correct:

PropertyChangedEventHandler handler = Volatile.Read(ref PropertyChanged);
if (handler != null)
{
    var e = new PropertyChangedEventArgs(propertyName);
    handler(this, e);
}

In reality though, the other pattern is so widely used that Microsoft would very likely never make a change to the compiler that would break so many applications.

One other point, is that you can actually add an empty dummy delegate to your events, like below:

public event PropertyChangedEventHandler PropertyChanged = delegate { };

This means you don't have to worry about any null checking, because the event has at least one handler attached to start with. This practice is controversial, though, since it would seem to incur a performance penalty (in theory you add an extra invocation to every event call). Personally I avoid using it, as I would rather suffer more bloated code than bloated runtime performance.

This blog post by Eric Lippert on the topic is a good read.

Couldn't you still assign `null` to the event? A quick test shows the assignment is valid. — BradleyDotNET, Jan 06 '15 at 19:58
@BradleyDotNET yes, I think you're right. You can only do that within the class that owns the event, so if you own the class then you at least have control over whether it gets set to null. But yes, still not 100% safe. — McGarnagle, Jan 06 '15 at 20:08

C# INotifyPropertyChanged handler

2 Answers2