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.

score 15 · Answer 1 · edited Apr 13 '16 at 15:05

15

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.

edited Apr 13 '16 at 15:05

manash

6,985
12
65
125

answered Sep 07 '09 at 13:48

Pop Catalin

61,751
23
87
115

[Interlocked.CompareExchange does not have an overload with two arguments](https://msdn.microsoft.com/en-us/library/system.threading.interlocked(v=vs.110).aspx) perhaps your line should read `return Interlocked.CompareExchange(ref m_LayoutSuspended, 1, 0) == 0;` – Nameless One Apr 14 '16 at 06:49
It has [also been pointed out to me](https://meta.stackoverflow.com/questions/320996/changed-edit-still-doesnt-compile#comment333601_320996) that changing it to `return Interlocked.Exchange(ref m_LayoutSuspended, 1) == 0;` will also do the trick. – Nameless One Apr 15 '16 at 13:35

score 10 · Answer 2 · answered Sep 07 '09 at 13:31

10

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

answered Sep 07 '09 at 13:31

Jon Skeet

1,421,763
867
9,128
9,194

@Jon: I'm curious, can you elaborate on "volatile doesn't mean quite what I thought it did"? – LukeH Sep 07 '09 at 13:35
Just to emphasize, a volatile long would **not** be safe (on a 32 bits system). – H H Sep 07 '09 at 13:49
3

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. – Pop Catalin Sep 07 '09 at 13:58
@Pop: Yours is definitely a cleaner answer, but would you agree that the volatile Boolean is as effective as the original code? – Jon Skeet Sep 07 '09 at 14:10
@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.) – Miral Oct 08 '10 at 05:59
updated link for the post mentioned in the second comment http://joeduffyblog.com/2008/06/13/volatile-reads-and-writes-and-timeliness/ – Adrian Rus Nov 20 '17 at 15:49

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

2 Answers2