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

Question

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.

Yevetteyew

Answer 1

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 CompareExchange 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.

Bobbibobbie

Interlocked.CompareExchange does not have an overload with two arguments perhaps your line should read return Interlocked.CompareExchange(ref m_LayoutSuspended, 1, 0) == 0; – Cohune

It has also been pointed out to me that changing it to return Interlocked.Exchange(ref m_LayoutSuspended, 1) == 0; will also do the trick. – Cohune

Answer 2

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...

Tippet

@Jon: I'm curious, can you elaborate on "volatile doesn't mean quite what I thought it did"? – Townsend

Just to emphasize, a volatile long would not be safe (on a 32 bits system). – Pun

volatile is only useful when a set of thread exclusively write to a field and a set of threads exclusively read a field, other than that it isn't of much use. – Bobbibobbie

@Pop: Yours is definitely a cleaner answer, but would you agree that the volatile Boolean is as effective as the original code? – Tippet

@Jon: Yes, the volatile bool should perform as safely as the original code (but more efficiently). It is not, however, safe for compare-then-change scenarios; for that you'd need Pop's code. (And either are more efficient than a lock.) – Demulcent

updated link for the post mentioned in the second comment joeduffyblog.com/2008/06/13/… – Acuate