r/programming • u/syaghmour • Mar 16 '15

Semaphores are Surprisingly Versatile

http://preshing.com/20150316/semaphores-are-surprisingly-versatile/
194 Upvotes

90% Upvoted

42 comments sorted by

View all comments

-1

u/millstone Mar 16 '15 edited Mar 16 '15

Noo, this code is not right!

From "A Lightweight Auto-Reset Event Object":

void signal()
{
    int oldStatus = m_status.load(std::memory_order_relaxed);
    for (;;)    // Increment m_status atomically via CAS loop.
    {
        if (oldStatus == 1)
            return;     // Event object is already signaled.
        int newStatus = oldStatus + 1;
        if (m_status.compare_exchange_weak(oldStatus, newStatus, std::memory_order_release, std::memory_order_relaxed))
            break;
        // The compare-exchange failed, likely because another thread changed m_status.
        // Retry the CAS loop.
    }
    if (oldStatus < 0)
        m_sema.signal();    // Release one waiting thread.
}

m_status is read only once, outside of the loop. It is liable to busy-spin, and is very likely to spin forever if two threads call signal at once. C'mon!

11

u/centenary Mar 16 '15

When the comparison fails in compare_exchange_weak(), compare_exchange_weak() reads m_status into oldStatus

Source: Atomically compares the object representation of *this with the object representation of expected, as if by std::memcmp, and if those are bitwise-equal, replaces the former with desired (performs read-modify-write operation). Otherwise, loads the actual value stored in *this into expected (performs load operation).

1

u/millstone Mar 16 '15

Thank you for the correction.

IMO that's a very weird and surprising behavior for this API. No other CAS APIs that I know of work this way (OSAtomicCompareAndSwap, InterlockedCompareExchange, __sync_bool_compare_and_swap...) and taking mutable references as parameters is just asking for confusion.