Can method inlining optimization cause race condit

Question

As seen in this question: Raising C# events with an extension method - is it bad?

I'm thinking of using this extension method to safely raise an event:

public static void SafeRaise(this EventHandler handler, object sender, EventArgs e)
{
    if (handler != null)
        handler(sender, e);
}

But Mike Rosenblum raise this concern in Jon Skeet's answer:

You guys need to add the [MethodImpl(MethodImplOptions.NoInlining)] attribute to these extension methods or else your attempt to copy the delegate to a temporary variable could be optimized away by the JITter, allowing for a null reference exception.

I did some test in Release mode to see if I could get a race condition when the extension method is not marked with NoInlining:

int n;
EventHandler myListener = (sender, e) => { n = 1; };
EventHandler myEvent = null;

Thread t1 = new Thread(() =>
{
    while (true)
    {
        //This could cause a NullReferenceException
        //In fact it will only cause an exception in:
        //    debug x86, debug x64 and release x86
        //why doesn't it throw in release x64?
        //if (myEvent != null)
        //    myEvent(null, EventArgs.Empty);

        myEvent.SafeRaise(null, EventArgs.Empty);
    }
});

Thread t2 = new Thread(() =>
{
    while (true)
    {
        myEvent += myListener;
        myEvent -= myListener;
    }
});

t1.Start();
t2.Start();

I ran the test for a while in Release mode and never had a NullReferenceException.

So, was Mike Rosenblum wrong in his comment and method inlining cannot cause race condition?

In fact, I guess the real question is, will SaifeRaise be inlined as:

while (true)
{
    EventHandler handler = myEvent;
    if (handler != null)
        handler(null, EventArgs.Empty);
}

or

while (true)
{
    if (myEvent != null)
        myEvent(null, EventArgs.Empty);
}

Answer 1

With correct code, optimizations should not change its semantics. Therefore no error can be introduced by the optimizer, if the error was not in the code already.

Answer 2

This is a memory model issue.

Basically the question is: if my code contains only one logical read, may the optimizer introduce another read?

Surprisingly, the answer is: maybe

In the CLR specification, nothing prevents optimizers from doing this. The optimization doesn't break single-threaded semantics, and memory access patterns are only guaranteed to be preserved for volatile fields (and even that's a simplication that's not 100% true).

Thus, no matter whether you use a local variable or a parameter, the code is not thread-safe.

However, the Microsoft .NET framework documents a different memory model. In that model, the optimizer is not allowed to introduce reads, and your code is safe (independent of the inlining optimization).

That said, using [MethodImplOptions] seems like a strange hack, as preventing the optimizer from introducing reads is only a side effect of not inlining. I'd use a volatile field or Thread.VolatileRead instead.

Answer 3

The problem wouldn't have been inlining the method - it would have been the JITter doing interesting things with memory access whether or not it was inlined.

However, I don't believe it is an issue in the first place. It was raised as a concern a few years back, but I believe that was regarded as a flawed reading of the memory model. There's only one logical "read" of the variable, and the JITter can't optimise that away such that the value changes between one read of the copy and the second read of the copy.

EDIT: Just to clarify, I understand exactly why this is causing a problem for you. You've basically got two threads modifying the same variable (as they're using captured variables). It's perfectly possible for the code to occur like this:

Thread 1                      Thread 2

                              myEvent += myListener;

if (myEvent != null) // No, it's not null here...

                              myEvent -= myListener; // Now it's null!

myEvent(null, EventArgs.Empty); // Bang!

This is slightly less obvious in this code than normally, as the variable is a captured variable rather than a normal static/instance field. The same principle applies though.

The point of the safe raise approach is to store the reference in a local variable which can't be modified from any other threads:

EventHandler handler = myEvent;
if (handler != null)
{
    handler(null, EventArgs.Empty);
}

Now it doesn't matter whether thread 2 changes the value of myEvent - it can't change the value of handler, so you won't get a NullReferenceException.

If the JIT does inline SafeRaise, it will be inlined to this snippet - because the inlined parameter ends up as a new local variable, effectively. The problem would only be if the JIT incorrectly inlined it by keeping two separate reads of myEvent.

Now, as to why you only saw this happen in debug mode: I suspect that with the debugger attached, there's far more room for threads to interrupt each other. Possibly some other optimisation occurred - but it didn't introduce any breakage, so that's okay.