Thread::waitForThreadToExit(0) waits forever

The documentation says:

[quote]bool Thread::waitForThreadToExit ( int timeOutMilliseconds ) const
Waits for the thread to stop.

This will waits until isThreadRunning() is false or until a timeout expires.

Parameters:
timeOutMilliseconds the time to wait, in milliseconds. If this value is less than zero, it will wait forever.
Returns:
true if the thread exits, or false if the timeout expires first.[/quote]

Since zero is not less than zero, waitForThreadToExit(0) should wait for 0ms—that is, just check and immediately return. But instead, it waits forever. If you look at the implementation, the problem is obvious: if (timeOutMilliseconds > 0 && --count < 0).

I’m not sure whether there’s ever a good reason to directly call waitForThreadToExit(0). There could be a memory barrier or even synchronization that makes this more useful than just calling isThreadRunning(), but as of the current top of tree (and the oldest version I have around, too), there isn’t.

But it definitely comes up when you have other functions that take a timeout parameter and pass it along to waitForThreadToExit, expecting the usual “0 = try and return immediately” meaning.

Ah, thanks very much for spotting that, it does just look like a slip of the keyboard, and you’re right that it should be >=

Unlikely that anyone would ever call it with 0, but I’ll get it fixed just in case!

Not that I’m anyone, but I called it with 0. Twice. So thanks!

Not directly, of course, but by passing a parameter or variable from somewhere else. Here’s a piece of toy code:

  bool waitForThreadToExit(const Thread &t, Time waituntil) {
    RelativeTime waitfor = waituntil - getCurrentTime();
    int millis = max(waitfor.inMilliseconds(), 0);
    t.waitForThreadToExit(millis);
  }

Of course this is a bad example (plus, it can trivially work around the problem by changing that 0 to a 1), but hopefully it’s enough to get the idea across.

Good point - that kind of thing could easily happen. Anyway, I fixed the code yesterday so should all be ok now.