This is Thread-Safe right? This is Thread-Safe right? multithreading multithreading

This is Thread-Safe right?


This is thread safe, right?

Suppose MAXIMUM is one, count is zero, and five threads call CheckForWork.

All five threads could verify that count is less than MAXIMUM. The counter would then be bumped up to five and five jobs would start.

That seems contrary to the intention of the code.

Moreover: the field is not volatile. So what mechanism guarantees that any thread will read an up-to-date value on the no-memory-barrier path? Nothing guarantees that! You only make a memory barrier if the condition is false.

More generally: you are making a false economy here. By going with a low-lock solution you are saving the dozen nanoseconds that the uncontended lock would take. Just take the lock. You can afford the extra dozen nanoseconds.

And even more generally: do not write low-lock code unless you are an expert on processor architectures and know all optimizations that a CPU is permitted to perform on low-lock paths. You are not such an expert. I am not either. That's why I don't write low-lock code.


No, if (_count >= MAXIMUM) return; is not thread safe.

edit: You'd have to lock around the read too, which should then logically be grouped with the increment, so I'd rewrite like

private int _count;private readonly Object _locker_ = new Object();public void CheckForWork() {    lock(_locker_)    {        if (_count >= MAXIMUM)            return;        _count++;    }    Task.Run(() => Work());}public void CompletedWorkHandler() {    lock(_locker_)    {        _count--;    }    ...}


That's what Semaphore and SemaphoreSlim are for:

private readonly SemaphoreSlim WorkSem = new SemaphoreSlim(Maximum);public void CheckForWork() {    if (!WorkSem.Wait(0)) return;    Task.Run(() => Work());}public void CompletedWorkHandler() {    WorkSem.Release();    ...}