kinderchocolate wrote:
So we're in the constructor and the start_routine might or might not be called before the constructor ends. We have a problem if it's called before the constructor returns because the derived version is not "ready".
Is this a bug?
No.
There is nothing that prevents you to call a virtual function from a c'tor
The only side effect is that it will not call the function of the class _derived_ from the one it is building, but will call the function defined in the same class, i.e. it is like the virtual call mechanism is disabled and the virtual function will behave like a normal method.
"In C++ the Base class will build its version of the virtual method table prior to entering its own construction. At this point a call to the virtual method will end up calling the Base version of the method"
kinderchocolate wrote:
So we're in the constructor and the start_routine might or might not be called before the constructor ends. We have a problem if it's called before the constructor returns because the derived version is not "ready".
Is this a bug?
No.
There is nothing that prevents you to call a virtual function from a c'tor
The only side effect is that it will not call the function of the class _derived_ from the one it is building, but will call the function defined in the same class, i.e. it is like the virtual call mechanism is disabled and the virtual function will behave like a normal method.
But this is really bad because it means that the MainThread and TimerThread objects can use the base class idle_loop function instead of the derived versions.
Here is a patch to make the bug easier to trigger:
Since I upgraded DroidFish to use stockfish 3, I have had several reports from users that stockfish just hangs. Up till now I have not been able to reproduced the problem, but it seems likely that this bug is responsible.
Having now taken a look at the Stockfish source. I have a few more comments:
1. Why is there a bunch of chess search stuff in Thread which should be in a derived class/struct instead? Currently, it looks like the single TimerThread instance gets all the chess definitions variables and uses none of them. Wouldn't it be better to have a Thread base class which had no chess stuff and have it and its derived classes include only the methods and instance variables which they actually used?
2. Using a pure virtual method (thus defining an interface) for idle_loop() allows the compiler to force compliance and leaves no room for confusion as to which version of idle_loop() is called by whom.
3. I see that in the space of a dozen or so lines, the exit identifier is used for a flag, for a method, and finally for the standard Unix program termination routine. Surely a better naming strategy could be used here.
So we cannot assume it is the wrong thing just because we learnt to do it in another way.
But you are not supposed to derive from std::thread (technically you can - which is why i still consider it flawed). That's a big difference.
Let's say I derive MyThread from Thread which physically starts it. How do you know that MyThread ctor finishes initializion of MyThread before the new physical thread starts to run, potentially starting to use member variable of MyThread which hasn't been potentially constructed yet?
The same applies to virtual destructor. ~Thread calls join() but the physical thread may still be running, accessing MyThread member variables that have been already destroyed.
petero2 wrote: But this is really bad because it means that the MainThread and TimerThread objects can use the base class idle_loop function instead of the derived versions.
OK, now I see it.
Yes this is a bug !
I have tried to quick write a patch with start() and finish(), but this adds some ugly code...I will try also the approach to wrap the idle_loop() in a separate class and derive from it and use composition to add that class to Thread, this should allow to preserve the c'tor and d'tor semantic but guarantee object are fully initialized when called.
void Thread::Finish(void)
{
if (pthread_join(*(pthread_t *) threadptr, 0))
Die("Thread::Finish", "pthread_join");
delete (pthread_t *) threadptr;
threadptr = 0; // To be consistent
}
Yet even in the unamended version, the Thread object can have multiple re-uses of sequential Start()/Finish() pairs of calls. This could be handy if the ctor/dtor does a lot of work that doesn't need repeated for sequential activations.
The isrunning instance variable is there for the outside world to see if the thread is still active. In the case of a Task instance, a true value means that the instance is still interested in processing incoming events on it's associated event list.
Generally, the isrunning variable is set to false by the Task instance when the task wants to say good-bye. For an instance of the interactive command processor task, the variable will be set false by an incoming EOF on the command input or by processing an exit command typed by the user. I'll probably add certain incoming signal kinds (i.e., SIG_HUP) as a third way of indicating a farewell request.
Over the years I've seen some really bad abuse of C++. I recall from long ago reading some C++ code which included the line:
Which to this day gives me the shivers considering how it could introduce all kinds of hard-to-debug randomness. Also, what if the instance was allocated on the stack or statically instead of on the heap? All because the guy who wrote it was too lazy to do it right in the first place.
For the reason you mentioned in your last paragraph, the Thread() constructor should initialize threadptr to 0, Thread::Start() should check whether threadptr is really 0 (otherwise Start() was called twice in a row), and Thread::Finish() should check whether threadptr is != 0. Of course you can't ASSERT() these conditions since Start() and Finish() can be called anywhere outside. Simplest would be to do nothing if the == 0 resp. != 0 conditions are not met.
I have hopefully fixed the race. The approach taken is to add a simple helper called new_thread() that mimics operator new, but first creates the object then starts it:
I have hopefully fixed the race. The approach taken is to add a simple helper called new_thread() that mimics operator new, but first creates the object then starts it:
template<typename T> T* new_thread() {
T* th = new T();
thread_create(th->handle, start_routine, th); // Will go to sleep
return th;
}
This allows to minimize code changes and, more importantly, to preserve the "initialization in c'tor" approach (called RAII by C++ people).
Fail. Epic fail.
This has nothing to do with RAII. Using create_thread() and delete_thread() is completely unsafe. Whenever you have an uncaught exception anywhere, the thread's destructor will not be called.
Proper RAII is to wrap the thread in a class (preferably a std::unique_ptr), where the constructor creates and the destructor cleans up. This way, whenever an exception happens after the thread has been created, the destructor is guaranteed to be called.
Yes. Memory leak happens if somehow exception is thrown after the new operator. But it doesn't really matter anyway because there's no point to continue executing if we can't start an engine thread.
