Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LightweightSemaphore weak-try? #12

Open
Caldfir opened this issue Jul 5, 2021 · 1 comment
Open

LightweightSemaphore weak-try? #12

Caldfir opened this issue Jul 5, 2021 · 1 comment

Comments

@Caldfir
Copy link

Caldfir commented Jul 5, 2021

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

return (oldCount > 0 && m_count.compare_exchange_strong(oldCount, oldCount - 1, std::memory_order_acquire));

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 Caldfir changed the title weak-try? LightweightSemaphore weak-try? Jul 5, 2021
@Caldfir
Copy link
Author

Caldfir commented Jul 5, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant