This is Thread-Safe right?

Question

Just checking... _count is being accessed safely, right?

Both methods are accessed by multiple threads.

private int _count;

public void CheckForWork() {
    if (_count >= MAXIMUM) return;
    Interlocked.Increment(ref _count);
    Task t = Task.Run(() => Work());
    t.ContinueWith(CompletedWorkHandler);
}

public void CompletedWorkHandler(Task completedTask) {
    Interlocked.Decrement(ref _count);
    // Handle errors, etc...
}

Shaftesbury

Literally went "BWAHAHAHAHAH!" at the title here. – Spermatium

I just want to point out, this is a pretty bad way to implement what's essentially a Task Factory with a max degree of parallelism. There are a whole host of problems, both on the design side (I'm guessing CheckForWork() is being called on a timer and in the "..." code; this is bad. Also, by default this is using the default Thread Pool implementation which is defeating a lot of what you are trying to do) and the implementation (The most obvious problem; if Work() throws an exception or someone forgets to call CompletedWorkHandler() you're going to starve your work queue). – Arlinearlington

Answer 1

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

Cypsela

Answer 2

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.

Coloration

I don't even know what you mean by low-lock code. What I was trying to accomplish was to somehow limit the number of concurrent work being done. I thought reads of value types where thread-safe. – Shaftesbury

@Unlimited071 That depends on what you mean by thread safe. Some operations can safely be done without locks, and some can't. Unless you know what you're doing, and how you expect it to be done, you can't really talk about whether the program is "safe" or not. Code can be safe if used in a given situation and be unsafe if used in another. Nothing is ever entirely thread save regardless of how it's used. – Ablebodied

@Unlimited071: Then you definitely should not be writing low-lock code! Low-lock code is what you're writing: code that tries to be threadsafe without taking any locks. I don't know what you mean by "trying to limit the number of concurrent work being done", but it sure sounds like "trying to avoid taking locks" to me. – Coloration

@Unlimited071: What ever gave you the idea that a read of a value type is safe? A read of a value type isn't even atomic unless it is an (aligned) integer or smaller! And atomicity is only one very small aspect of thread safety. You're doing an atomic read and an atomic increment; since those are two things, that operation put together is not atomic, and it certainly doesn't make a barrier on the return path. – Coloration

@Unlimited071 The term low-lock code is used when you try to synchronize access without using the 'lock' feature of the language and using something else e.g. 'Interlocked.*' like in your code for synchronization. Eric is suggesting that unless you are an expert in using low lock techniques, it is better to use 'lock' keyword to implement synchronization. Code using the 'lock' keyword is easier to write, maintain and reason about. – Licence

The potential downside of using 'lock' is that it may cause performance issues depending on the nature of your program. If you suspect that, make sure to profile your code first and try to investigate in to low-level lock techniques. – Licence

@EricLippert Is there any way anyone could prove that just the read of some value type is not atomic? Does it ever matter that a single read of a value type is not atomic? Or does it only ever matter that the larger operation is not atomic? – Coburn

@BlueMonkMN: I don't understand the question. Are you asking "consider three propositions: (1) writes are atomic, reads are not, (2) reads are atomic, writes are not, (3) neither are atomic. Is there a set of observations from which we can logically prove which of the three propositions is true?" I don't know the answer to that question nor do I particularly care to work out the nature of proof in a counterfactual world. I am asserting to you that proposition 3 is true for 64 bit integers, and if you don't want to take my word for it, I can't really provide you with "proof". – Coloration

@BlueMonkMN: Yes, it matters. In the 32 bit framework, reads and writes of long are not guaranteed atomic. So absent synchronization one thread could be reading while another is writing, and end up with the high 32 bits of the previous value and the low 32 bits of the new value. I'll let you decide if that's a potential problem. – Caroche

@EricLippert My point/question was only about the purpose of your point that reads were not atomic. Why does it matter that reads are not atomic if you can't prove it in any way other than to do something else that's intrinsically not atomic (assigning two members of a value type separately, for example, which is obviously not atomic). Even when talking about long integers in the 32-bit framework, you have to fall back on writes also being not atomic. The clearer point is that "accessing a value type is not atomic in general," but it seems harder to prove that only reads are not atomic. – Coburn

@BlueMonkMN: First: if you're only reading then you've already solved the thread safety problem. Second, ok, let's suppose that reads are non-atomic and writes are atomic. Now we're reasoning from falsehood, but let's go with it. You write DEADBEEFBAADF00D atomically to a long. You read DEADBEEF non-atomically, then write atomically 0000000000000000, then read 00000000 non-atomically, and you've read DEADBEEF00000000, which is a value that was never written. Non-atomic reads are sufficient to make accessing 64 bit longs unsafe, even if writes are atomic. Which they are not. – Coloration

@BlueMonkMN: The question I thought you were asking is "is there a way to prove that reads and writes are both non-atomic?", ie, is there some observation we can make that falsifies or confirms that hypothesis? And my answer to that is "I don't know and I don't care; reads and writes are non-atomic, so let's reason about facts rather than counterfactuals." – Coloration

@EricLippert The DEADBEEFBAADFOOD example relies on us knowing that writes are atomic. The problem is, assuming we don't know the nature of writes, if we demonstrate non-atomicity of reads, we can't conclude for certain that the result occurred because the read wasn't atomic or because the write wasn't atomic. And that was my point/question. It seems there's little purpose in distinguishing non-atomic reads from non-atomic writes because you can't prove whether it's the read or the write that's non-atomic. I accept that both are non-atomic, but I think the read doesn't matter. – Coburn

@BlueMonkMN: I understand your point; I don't understand why you're making it. Reads and writes are non-atomic, end of story. The fact that in a world where we didn't know that, it would be difficult to deduce which if either is necessarily non-atomic is uninteresting; we don't live in that world. If we lived on the lone planet in a solar system inside a big dust cloud it would be very difficult to tell if the sun went around the planet or if the planet spun. We don't live on such a planet, and we know that the earth goes around the sun, so the counterfactual is irrelevant. – Coloration

The more or less sane definition of "thread-safe" is "interleaving the threads doesn't bring in the non-ndeterminism to the result". – Suspicion

@Joker_vD: That's not a very strong definition. Consider for example two variables containing counters that are being incremented and decremented by multiple threads while other threads are observing. It is perfectly legal for a third thread to observe two variables mutating x first then y, and for a fourth thread to observe the mutation to y first and then x, nondeterministically. But most people would say that the operations are threadsafe provided that the counters never lose increments or decrements. Again, we need to carefully describe what we mean by "thread safe". – Coloration

@EricLippert Well, basically we have to decide what manifestations of non-determinism are acceptable, and when/where. Actually enforcing such policies is still hard, because accidental slips are easy to made and hard to spot. – Suspicion

Answer 3

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();
    ...
}

Caroche

Answer 4

No, what you have is not safe. The check to see if _count >= MAXIMUM could race with the call to Interlocked.Increment from another thread. This is actually really hard to solve using low-lock techniques. To get this to work properly you need to make a series of several operations appear atomic without using a lock. That is the hard part. The series of operations in question here are:

• Read _count
• Test _count >= MAXIMUM
• Make a decision based on the above.
• Increment _count depending on the decision made.

If you do not make all 4 of these steps appear atomic then there will be a race condition. The standard pattern for performing a complex operation without taking a lock is as follows.

public static T InterlockedOperation<T>(ref T location)
{
  T initial, computed;
  do
  {
    initial = location;
    computed = op(initial); // where op() represents the operation
  } 
  while (Interlocked.CompareExchange(ref location, computed, initial) != initial);
  return computed;
}

Notice what is happening. The operation is repeatedly performed until the ICX operation determines that the initial value has not changed between the time it was first read and the time the attempt was made to change it. This is the standard pattern and the magic all happens because of the CompareExchange (ICX) call. Note, however, that this does not take into account the ABA problem.¹

What you could do:

So taking the above pattern and incorporating it into your code would result in this.

public void CheckForWork() 
{
    int initial, computed;
    do
    {
      initial = _count;
      computed = initial < MAXIMUM ? initial + 1 : initial;
    }
    while (Interlocked.CompareExchange(ref _count, computed, initial) != initial);
    if (replacement > initial)
    {
      Task.Run(() => Work());
    }
}

Personally, I would punt on the low-lock strategy entirely. There are several problems with what I presented above.

• This might actually run slower than taking a hard lock. The reasons are difficult to explain and outside the scope of my answer.
• Any deviation from what is above will likely cause the code to fail. Yes, it really is that brittle.
• It is hard to understand. I mean look at it. It is ugly.

What you should do:

Going with the hard lock route your code might look like this.

private object _lock = new object();
private int _count;

public void CheckForWork() 
{
  lock (_lock)
  {
    if (_count >= MAXIMUM) return;
    _count++;
  }
  Task.Run(() => Work());
}

public void CompletedWorkHandler() 
{
  lock (_lock)
  {
    _count--;
  }
}

Notice that this is much simpler and considerably less error prone. You may actually find that this approach (hard lock) is actually faster than what I showed above (low lock). Again, the reason is tricky and there are techniques that can be used to speed things up, but it outside the scope of this answer.

---

¹The ABA problem is not really an issue in this case because the logic does not depend on _count remaining unchanged. It only matters that its value is the same at two points in time regardless of what happened in between. In other words the problem can be reduced to one in which it seemed like the value did not change even though in reality it may have.

Whether

Well, I should have omitted Task.Run. What I tried to say was the method was queuing some work that I'm trying to limit. – Shaftesbury

Answer 5

Define thread safe.

If you want to ensure that _count will never be greater than MAXIMUM, than you did not succeed.

What you should do is lock around that too:

private int _count;
private object locker = new object();

public void CheckForWork() 
{
    lock(locker)
    {
        if (_count >= MAXIMUM) return;
        _count++;
    }
    Task.Run(() => Work());
}

public void CompletedWorkHandler() 
{
    lock(locker)
    {
        _count--;
    }
    ...
}

You might also want to take a look at The SemaphoreSlim class.

Delighted

Thanks for the SemaphoreSlim class link, It looks like it could help with what I'm trying to accomplish – Shaftesbury

Answer 6

you can do the following if you don't want to lock or move to a semaphore:

if (_count >= MAXIMUM) return; // not necessary but handy as early return
if(Interlocked.Increment(ref _count)>=MAXIMUM+1)
{
    Interlocked.Decrement(ref _count);//restore old value
    return;
}
Task.Run(() => Work());

Increment returns the incremented value on which you can double check whether _count was less than maximum, if the test fails then I restore the old value

Leta

Your code allows live lock to occur, where 40 threads are always between having incremented too far and having fixed that by decrementing. No work can get done while that is the case, because you get false positives on the 'is there too much work?'. – Wield

@Strilanc the decrement in the if block will only reduce to MAXIMUM so the early return will happen, also the first MAXIMUM threads will be able to do work, and when a thread is done then an increment will return MAXIMUM-1 for some thread allowing it to run, beside livelock is always a danger when choosing for atomics. – Leta

Strilanc is right. This is really hard to visualize. Let me try to explain a different way. Imagine 400 threads all competing for the increment and decrement tandem. Now, assume that there is a 10% probability that any one thread will be between the two calls at any given moment. If the distribution and probabilities remain steady (which might occur under a heavy load) then we infer that there are 400 * 0.1 = 40 threads always in this half-baked state. If 40 threads are always in this state then _count must be >= 40 at all times even if no work is currently being done. Live locked! – Whether

@BrianGideon I see but you really shouldn't have 400 threads competing for a limited reasource, but the solution of Brian will fix it – Leta