preshing / cpp11-on-multicore

Various synchronization primitives for multithreaded applications in C++11.
zlib License
517 stars 102 forks source link

LightweightSemaphore weak-try? #12

Open Caldfir opened 3 years ago

Caldfir commented 3 years ago

Reading through the code here, I noted the following implementation of LightweightSemaphore::tryWait():

https://github.com/preshing/cpp11-on-multicore/blob/41ac9c73dfa32e2fbc0af6668358092fafdfcae4/common/sema.h#L201

If I am not mistaken, it is possible for another thread to signal between the load and the compare_exchange_strong which would result in a spurious failure. I think this is commonly referred to as a "weak-try" but if that was the intent then using compare_exchange_weak would be sufficient. I only see this code being used in one single place in this project, and in that instance the strong/weak distinction wouldn't be relevant.

I'm certainly no expert on the topic of C++11 atomic operations, so I could be mistaken.

Caldfir commented 3 years ago

To clarify, the issue is that it seems to be possible for tryWait to return false while the semaphore is positive because CAS could fail from a signal() in another thread. The only resolution I can think of would be to loop:

int oldCount;
while (1)
{
  oldCount = m_count.load(std::memory_order_relaxed);
  if (oldCount <= 0) return false;
  if (m_count.compare_exchange_weak(oldCount, oldCount - 1, std::memory_order_acquire)) return true;
}

But while(1) in a method called try... sounds obscene, even if in even the worst case this would only actually loop very infrequently.