Threads complete but loop doesn't end

Question

I wrote some threading code with what seems to be an incorrect assumption, that integers were thread-safe. Now it seems that although they are, my use of them is NOT thread safe. I'm using a global integer ThreadCount to hold the number of threads. During thread create, I increment the ThreadCount. During thread destroy, I decrement it. After all threads are done being created, I wait for them to be done (ThreadCount should drop to 0) and then write my final report and exit.

Sometimes (5%) though, I never get to 0, even though a post-mortem examination of my log shows that all threads did run and complete. So all signs point to ThreadCount getting trampled. I have been telling myself that this is impossible since it's an integer, and I'm just using inc/dec.

Here some relevant code:

var  // global
  ThreadCount : integer;            // Number of active threads
...

constructor TTiesUpsertThread.Create(const CmdStr : string);
begin
  inherited create(false);
  Self.FreeOnTerminate := true;
...
  Inc(ThreadCount);      // Number of threads created.  Used for throttling.
end;

destructor TTiesUpsertThread.Destroy;
begin
  inherited destroy;
  Dec(ThreadCount);     // When it reaches 0, the overall job is done.
end;

...
//down at the end of the main routine:

    while (ThreadCount > 0) do   // Sometimes this doesn't ever end.  
    begin
      SpinWheels('.'); // sleeps for 1000ms and writes dots... to console 
    end;

I THINK my problem is with inc/dec. I think I'm getting collisions where an two or more dec() hit at the same time and both read the same value, so they replace it with the same value. ex: ThreadCount = 5, and two threads end at the same time, both read 5, replace with 4. But the new value should be 3.

This never runs into trouble in our test environment, (different hardware, topology, load, etc..) so I'm looking for confirmation that this is likely the problem, before I try to "sell" this solution to the business unit.

If this is my problem, do I use a critical selection to protect the inc/dec?
Thanks for taking a look.

Answer 1

If multiple threads modify the variable without protection then yes you have a data race. If two threads attempt to increment or decrement at the same instance then what happens is:

1. The variable is read into a register.
2. The modification is made in the register.
3. The new value is written back to the variable.

That read/modify/write is not atomic. If you have two threads executing at the same time then you have the canonical data race.

• Thread 1 reads the value, N say.
• Thread 2 reads the value, the same value as was read by thread 1, N.
• Thread 1 writes N+1 to the variable.
• Thread 2 writes N+1 to the variable.

And instead of the variable being incremented twice, it is incremented only once.

In this case there's no need for a full blown critical section. Use InterlockedIncrement to perform lock-free, thread safe modification.