C# Could write instructions be reordered from finally block into try block?

Question

In an article about A scalable reader/writer scheme with optimistic retry there is a code example:

using System;
using System.Threading;

public class OptimisticSynchronizer
{
    private volatile int m_version1;
    private volatile int m_version2;

    public void BeforeWrite() {
        ++m_version1;
    }

    public void AfterWrite() {
        ++m_version2;
    }

    public ReadMark GetReadMark() {
        return new ReadMark(this, m_version2);
    }

    public struct ReadMark
    {
        private OptimisticSynchronizer m_sync;
        private int m_version;

        internal ReadMark(OptimisticSynchronizer sync, int version) {
            m_sync = sync;
            m_version = version;
        }

        public bool IsValid {
            get { return m_sync.m_version1 == m_version; }
        }
    }

    public void DoWrite(Action writer) {
        BeforeWrite();
        try {
            writer(); // this is inlined, method call just for example
        } finally {
            AfterWrite();
        }
    }

    public T DoRead<T>(Func<T> reader) {
        T value = default(T);

        SpinWait sw = new SpinWait();
        while (true) {
            ReadMark mark = GetReadMark();

            value = reader();

            if (mark.IsValid) {
                break;
            }

            sw.SpinOnce();
        }

        return value;
    }
}

If I make m_version1 and m_version2 not volatile but then use the code:

public void DoWrite(Action writer) {
    Thread.MemoryBarrier(); // always there, acquiring write lock with Interlocked method
    Volatile.Write(ref m_version1, m_version1 + 1); // NB we are inside a writer lock, atomic increment is not needed
    try {
        writer();
    } finally {
        // is a barrier needed here to avoid the increment reordered with writer instructions?
        // Volatile.Write(ref m_version2, m_version2 + 1); // is this needed instead of the next line?
        m_version2 = m_version2 + 1; // NB we are inside a writer lock, atomic increment is not needed
        Thread.MemoryBarrier(); // always there, releasing write lock with Interlocked method
    }
}

Could instructions from line m_version2 = m_version2 + 1 be reordered from finally into try block? It is important that a writer finishes before m_version2 is incremented.

Logically finally is executed after try, but the finally block is not mentioned in the list of implicit memory barriers. It would be quite confusing if instructions from finally could be moved before the ones from try, but CPU optimizations at instructions level are still a black magic for me.

I could put Thread.MemoryBarrier(); before the line m_version2 = m_version2 + 1 (or use Volatile.Write), but the question is if this is really needed?

The MemoryBarriers shown in the example are implicit and generated by Interlocked methods of a writer lock, so they are always there. The danger is that a reader could see m_version2 incremented before the writer finishes.

I have read http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-335.pdf after posting this. It looks like Volatile.Write is needed, there is no special treatment of `finally` (section I.12.6-7). Unless I missed something very subtle about CERs. — V.B., Apr 06 '17 at 02:06

Valery Petrov · Answer 1 · 2017-12-25T14:57:55.127

I didn't find anything in specification that will restrict it, so I checked it with an ARM CPU device (using Xamarin, have to check it on Core CLR)...
One thread was executing this code:

try
{
    person = new Person();
}
finally
{
    isFinallyExecuted = true;
}

And the second thread was waiting for isFinallyExecuted to be true with this code:

while (!Volatile.Read(ref isFinallyExecuted))
    ;

Then the second thread was executing the following code:

if (!person.IsInitialized())
{
    failCount++;
    Log.Error("m08pvv", $"Reordered from finally: {failCount}, ok: {okCount}");
}
else
{
    okCount++;
}

IsInitialized method checks that all fields are properly setted, so it returns false for partially constructed objects.

This is what I got in the log:

12-25 17:00:55.294: E/m08pvv(11592): Reordered from finally: 48, ok: 682245
12-25 17:00:56.750: E/m08pvv(11592): Reordered from finally: 49, ok: 686534
12-25 17:00:56.830: E/m08pvv(11592): Reordered from finally: 50, ok: 686821
12-25 17:00:57.310: E/m08pvv(11592): Reordered from finally: 51, ok: 688002
12-25 17:01:12.191: E/m08pvv(11592): Reordered from finally: 52, ok: 733724
12-25 17:01:12.708: E/m08pvv(11592): Reordered from finally: 53, ok: 735338
12-25 17:01:13.722: E/m08pvv(11592): Reordered from finally: 54, ok: 738839
12-25 17:01:25.240: E/m08pvv(11592): Reordered from finally: 55, ok: 775645

Which means that for 775645 successful runs of that code, 55 times I got isFinallyExecuted equals true and partially constructed object. This is possible because I don't use Volatile.Write to store new Person() or volatile keyword on a person.

So, if you have some data races, you will face them.

C# Could write instructions be reordered from finally block into try block?

1 Answers1