.NET 4.7 JIT creates infinite loop, where .NET 4.6 JIT did not

[FlsZen]

Building the below code in VS Release mode results in different behavior when running the resulting binary in .NET 4.6 vs 4.7.

static void Main(string[] args)
{
    var c = new ChangeAFlagLater();
    while (!c.Flag) ;
}

class ChangeAFlagLater
{
    public ChangeAFlagLater()
    {
        Task.Delay(1000).ContinueWith(_ => Flag = true);
    }

    public bool Flag;
}

When running with 4.6, the while loop exits after the other thread changes Flag, but with .NET 4.7 it gets stuck looping forever. It's clear in the 4.7 disassembly that Flag isn't being checked again after entering the loop:

00007FF90031049F  movzx       eax,byte ptr [rsi+8]
00007FF9003104A3  test        eax,eax  
00007FF9003104A5  je          00007FF9003104A3  

Obviously, Flag should be marked volatile since it's being changed by another thread, and doing so prevents the infinite loop (je goes to 49F instead). Unfortunately, we found this in a production release running on a machine that had .NET 4.7 installed on it through Windows Updates, so it's a rather sneaky change in behavior. We hadn't noticed it yet on our dev machines because we mostly run in debug/without compiler optimizations.

Is this change in JIT considered a bug or is it a known issue or something?

[jnm2]

From what I know, it's a fluke that that code ever worked. Like you say, you should use Volatile.Read(ref c.Flag) and Volatile.Write(ref Flag, true). I think it's a valid optimization to observe that the thread you're on never changes the value of c.Flag and therefore it isn't worth checking. Even without that optimization, there's no guarantee that the thread will update its cache of the value.

But further, you're burning CPU with that busy loop, possibly starving other threads of CPU on a single-threaded system. It's much better to use a signaling primitive like ManualResetEventSlim. You should be doing something similar to:

static void Main(string[] args)
{
    var c = new ChangeAFlagLater();
    c.WaitForChange();
}

class ChangeAFlagLater
{
    public ChangeAFlagLater()
    {
        Task.Delay(1000).ContinueWith(_ => mres.Set());
    }

    private readonly ManualResetEventSlim mres = new ManualResetEventSlim();
    public void WaitForChange() => mres.Wait();
}

Or if the ChangeAFlagLater class itself shouldn't be responsible to notify you when its flag changes, whoever does cause the flag to change is responsible to provide a ManualResetEventSlim or AutoResetEvent or other means of letting other threads wait nicely.

[FlsZen]

The author of the loop told me that it was a testing hack and he intended to use a proper wait before committing. My fix was to have it await the completion of the Task that was responsible for changing the flag.

After doing some more searching, it looks like I have my premise backwards. There are a few examples of this optimization in .NET versions before 4.6. So 4.6 looks to be the outlier where this loop didn't result in an infinite loop, and we managed to add the code after switching the app to build to 4.6.

[AronParker]

Is the continuation executed on a different thread? If so then this behaviour is totally fine and the optimizer does its job.

[FlsZen]

@AronParker Yes.