Interlocked used to increment/mimick a boolean, is this safe?

Question

Hi,

I'm just wondering whether this code that a fellow developer (who has since left) is OK, I think he wanted to avoid putting a lock. Is there a performance difference between this and just using a straight forward lock?

    private long m_LayoutSuspended = 0;
    public void SuspendLayout()
    {
        Interlocked.Exchange(ref m_LayoutSuspended, 1);
    }

    public void ResumeLayout()
    {
        Interlocked.Exchange(ref m_LayoutSuspended, 0);
    }

    public bool IsLayoutSuspended
    {
        get { return Interlocked.Read(ref m_LayoutSuspended) != 1; }
    }

I was thinking that something like that would be easier with a lock? It will indeed be used by multiple threads, hence why the use of locking/interlocked was decided.

Answer 1

Personally I'd use a volatile Boolean:

private volatile bool m_LayoutSuspended = false;
public void SuspendLayout()
{
    m_LayoutSuspended = true;
}

public void ResumeLayout()
{
    m_LayoutSuspended = false;
}

public bool IsLayoutSuspended
{
    get { return m_LayoutSuspended; }
}

Then again, as I've recently acknowledged elsewhere, volatile doesn't mean quite what I thought it did. I suspect this is okay though :)

Even if you stick with Interlocked, I'd change it to an int... there's no need to make 32 bit systems potentially struggle to make a 64 bit write atomic when they can do it easily with 32 bits...

Answer 2

Yes what you are doing is safe from a race point of view reaching the m_LayoutSuspended field, however, a lock is required for the following reason if the code does the following:

if (!o.IsLayoutSuspended)  // This is not thread Safe .....
{
  o.SuspendLayout();   // This is not thread Safe, because there's a difference between the checck and the actual write of the variable a race might occur.
  ...
  o.ResumeLayout();
}

A safer way, that uses compare exchange to make sure no race conditions have occurred:

private long m_LayoutSuspended = 0;
public bool SuspendLayout()
{
    return Interlocked.CompareExchange(ref m_LayoutSuspended, 1) == 0;
}

if (o.SuspendLayout()) 
{
  ....
  o.ResumeLayout();
}

Or better yet simply use a lock.