I notice that it does do that in the destructor of Thread, and I’m wondering why you’d ever call signalThreadShouldExit() and not want to call notify() immediately thereafter?
Well, it calls notify() in stopThread to prevent deadlock because the caller won’t have the opportunity to do so themselves. But it’s possible that if you’re calling signalThreadShouldExit() directly, you might not always want it to interrupt a wait()… I’m sure I could do what you suggest without actually breaking anyone’s code, but it feels like it’d be giving that method more responsibility than it should have.
I’m very sympathetic to the “method should do one thing” argument.
But the reason I’m suggesting it is that I think it’s a trap for the unwary - but more, I don’t logically see why you would ever want to call signalThreadShouldExit and not call notify()!
I believe that the semantics of signalThreadShouldExit is that the thread should exit “as soon as possible” (which is why it’s “nice” to periodically check threadShouldExit() if you have a long loop).
If you are in a wait(), you are meaning that your thread is doing nothing until something exterior notifies it, or until any non-negative wait time expires. I can conceive of no logical reason you would not want to be woken up instantly when your thread is requested to exit.
The reason it’s a trap is that calling signalThreadShouldExit() and not calling notify() can easily introduce a race condition = consider the case where sometimes the thread is running when you call signalThreadShouldExit(), but sometimes it’s wait(-1). This could lead to nasty issues like loss of liveness or file handles being left open; and because it’s a race condition, it might be very intermittent and difficult to track down or disappear and reappear from build to build!
This leads to Thread::wait(), which doesn’t check threadShouldExit_ before it waits.
I’m a bit more hesitant about this one, because it seems like a method that a zillion people call, but I still can’t see why you would want to wait under any circumstances if threadShouldExit_ were true. I believe this leads to a potential race condition for much the same reason as above and I don’t see the benefit in it, nor how you could break something by adding this check…
I think that because signalThreadShouldExit is a name that describes one specific duty, i.e. setting a flag, then it does need to stick to that, and not have any surprising side-effects. If it was called “makeThreadStopAsSoonAsPossible”, then it’d be ok to make it a bit more pro-active. It’s quite possible that you might want to tell the thread to stop as soon as it’s convenient, without interrupting anything, e.g. if you’re using wait() as a timer rather than as something that’s waiting for a particular event.
Interesting point about the race condition, but calling signalThreadShouldExit() just sends a signal, it doesn’t make any kind of claim that your thread will actually take any notice of it… Even if wait()/notify() was made absolutely foolproof, there are an infinite number of other ways to write a thread that gets stuck! But even if you did forget to call notify() yourself, when you eventually called stopThread(), it would call notify() and manage to stop correctly.
Perhaps a note in the comments, then (“You probably want to call notify just in case”?)
Yep, that’d probably be a helpful comment.