Fix AbandonableTask being utterly broken

src/tasks/AbandonableTask.cpp

@@ -2,79 +2,18 @@
 
 #include "Platform.h"
 
-#ifdef DEBUG
+void AbandonableTask::doWork_wrapper()
-# define DEBUGTHREADS
-// * Uncomment above if AbandonableTasks cause issues.
-// * Three outputs should be possible:
-//   * The task is properly finished:
-//       AbandonableTask @ [ptr] ctor
-//       AbandonableTask @ [ptr] created
-//       ...
-//       AbandonableTask @ [ptr] finished
-//       AbandonableTask @ [ptr] joined
-//       AbandonableTask @ [ptr] dtor
-//   * The task is abandoned but its thread has finished:
-//       AbandonableTask @ [ptr] ctor
-//       AbandonableTask @ [ptr] created
-//       ...
-//       AbandonableTask @ [ptr] abandoned
-//       AbandonableTask @ [ptr] joined
-//       AbandonableTask @ [ptr] dtor
-//   * The task is abandoned before its thread has finished:
-//       AbandonableTask @ [ptr] ctor
-//       AbandonableTask @ [ptr] created
-//       ...
-//       AbandonableTask @ [ptr] abandoned
-//       ...
-//       AbandonableTask @ [ptr] detached
-//       AbandonableTask @ [ptr] dtor
-// * Anything other than those means something is broken.
-#endif
-
-#ifdef DEBUGTHREADS
-# include <iostream>
-#endif
-
-void AbandonableTask::Start()
 {
-	thDone = false;
+	Task::doWork_wrapper();
-	done = false;
+	pthread_cond_signal(&done_cv);
-	thAbandoned = false;
-	progress = 0;
-	status = "";
-	before();
-	pthread_mutex_init (&taskMutex, NULL);
-	pthread_create(&doWorkThread, 0, &AbandonableTask::doWork_helper, this);
 
-#ifdef DEBUGTHREADS
+	pthread_mutex_lock(&taskMutex);
-	std::cerr << "AbandonableTask @ " << this << " created" << std::endl;
+	bool abandoned = thAbandoned;
-#endif
+	pthread_mutex_unlock(&taskMutex);
-}
-
-TH_ENTRY_POINT void * AbandonableTask::doWork_helper(void * ref)
-{
-	Task::doWork_helper(ref);
-
-	AbandonableTask *task = (AbandonableTask *)ref;
-	pthread_mutex_lock(&task->taskMutex);
-	pthread_cond_signal(&task->done_cv);
-	bool abandoned = task->thAbandoned;
-	pthread_mutex_unlock(&task->taskMutex);
 	if (abandoned)
 	{
-		pthread_detach(task->doWorkThread);
+		delete this;
-		pthread_mutex_destroy(&task->taskMutex);
-
-#ifdef DEBUGTHREADS
-		std::cerr << "AbandonableTask @ " << ref << " detached" << std::endl;
-#endif
-
-		// We've done Task::~Task's job already.
-		task->done = true;
-
-		delete task;
 	}
-	return NULL;
 }
 
 void AbandonableTask::Finish()
@@ -87,13 +26,11 @@ void AbandonableTask::Finish()
 	pthread_mutex_unlock(&taskMutex);
 
 	// Poll to make sure that the rest of the Task knows that it's
-	// done, not just us.
+	// done, not just us. This has to be done because the thread that started
+	// the AbandonableTask may or may not call Poll before calling Finish.
+	// This may call callbacks.
 	Poll();
 
-#ifdef DEBUGTHREADS
-	std::cerr << "AbandonableTask @ " << this << " finished" << std::endl;
-#endif
-
 	delete this;
 }
 
@@ -106,65 +43,32 @@ void AbandonableTask::Abandon()
 		// If thDone is true, the thread has already finished. We're
 		// not calling Poll because it may call callbacks, which
 		// an abandoned task shouldn't do. Instead we just delete the
-		// AbandonableTask after unlocking the mutex, which will
+		// AbandonableTask after unlocking the mutex.
-		// take care of the thread if another Poll hasn't already
-		// (i.e. if done is false despite thDone being true).
 		delete_this = true;
 	}
 	else
 	{
 		// If at this point thDone is still false, the thread is still
 		// running, meaning we can safely set thAbandoned and let
-		// AbandonableTask::doWork_helper detach the thread
+		// AbandonableTask::doWork_wrapper delete the AbandonableTask later.
-		// and delete the AbandonableTask later.
 		thAbandoned = true;
 	}
 	pthread_mutex_unlock(&taskMutex);
 
-#ifdef DEBUGTHREADS
-	std::cerr << "AbandonableTask @ " << this << " abandoned" << std::endl;
-#endif
-
 	if (delete_this)
 	{
 		delete this;
 	}
 }
 
-void AbandonableTask::Poll()
+AbandonableTask::AbandonableTask() :
-{
+	thAbandoned(false)
-#ifdef DEBUGTHREADS
-	bool old_done = done;
-#endif
-	Task::Poll();
-#ifdef DEBUGTHREADS
-	if (done != old_done)
-	{
-		std::cerr << "AbandonableTask @ " << this << " joined" << std::endl;
-	}
-#endif
-}
-
-AbandonableTask::AbandonableTask()
 {
 	pthread_cond_init(&done_cv, NULL);
-#ifdef DEBUGTHREADS
-	std::cerr << "AbandonableTask @ " << this << " ctor" << std::endl;
-#endif
 }
 
 AbandonableTask::~AbandonableTask()
 {
-#ifdef DEBUGTHREADS
-	if (!done)
-	{
-		// Actually it'll be joined later in Task::~Task, but the debug
-		// messages look more consistent this way.
-		std::cerr << "AbandonableTask @ " << this << " joined" << std::endl;
-	}
-
-	std::cerr << "AbandonableTask @ " << this << " dtor" << std::endl;
-#endif
 	pthread_cond_destroy(&done_cv);
 }
 

src/tasks/AbandonableTask.h

@@ -8,16 +8,14 @@ class AbandonableTask : public Task
 	pthread_cond_t done_cv;
 	
 public:
-	void Start() override;
 	void Finish();
 	void Abandon();
-	void Poll() override;
 	AbandonableTask();
 	virtual ~AbandonableTask();
 
 protected:
+	void doWork_wrapper() override;
 	bool thAbandoned;
-	TH_ENTRY_POINT static void * doWork_helper(void * ref);
 };
 
 #endif /* ABANDONABLETASK_H_ */

src/tasks/Task.cpp

@@ -11,14 +11,11 @@ void Task::AddTaskListener(TaskListener * listener)
 
 void Task::Start()
 {
-	thDone = false;
-	done = false;
-	progress = 0;
-	status = "";
-	//taskMutex = PTHREAD_MUTEX_INITIALIZER;
 	before();
-	pthread_mutex_init (&taskMutex, NULL);
+	// This would use a lambda if we didn't use pthreads and if I dared omit
+	// the TH_ENTRY_POINT from the function type.
 	pthread_create(&doWorkThread, 0, &Task::doWork_helper, this);
+	pthread_detach(doWorkThread);
 }
 
 int Task::GetProgress()
@@ -83,24 +80,25 @@ void Task::Poll()
 		if(newDone!=done)
 		{
 			done = newDone;
-
-			pthread_join(doWorkThread, NULL);
-			pthread_mutex_destroy(&taskMutex);
-
 			after();
-
 			notifyDoneMain();
 		}
 	}
 }
 
+Task::Task() :
+	progress(0),
+	done(false),
+	thProgress(0),
+	thDone(false),
+	listener(NULL)
+{
+	pthread_mutex_init(&taskMutex, NULL);
+}
+
 Task::~Task()
 {
-	if(!done)
+	pthread_mutex_destroy(&taskMutex);
-	{
-		pthread_join(doWorkThread, NULL);
-		pthread_mutex_destroy(&taskMutex);
-	}
 }
 
 void Task::before()
@@ -123,13 +121,18 @@ void Task::after()
 
 }
 
-TH_ENTRY_POINT void * Task::doWork_helper(void * ref)
+void Task::doWork_wrapper()
 {
-	bool newSuccess = ((Task*)ref)->doWork();
+	bool newSuccess = doWork();
-	pthread_mutex_lock(&((Task*)ref)->taskMutex);
+	pthread_mutex_lock(&taskMutex);
-	((Task*)ref)->thSuccess = newSuccess;
+	thSuccess = newSuccess;
-	((Task*)ref)->thDone = true;
+	thDone = true;
-	pthread_mutex_unlock(&((Task*)ref)->taskMutex);
+	pthread_mutex_unlock(&taskMutex);
+}
+
+TH_ENTRY_POINT void *Task::doWork_helper(void *ref)
+{
+	((Task *)ref)->doWork_wrapper();
 	return NULL;
 }
 

src/tasks/Task.h

@@ -17,7 +17,7 @@ public:
 	String GetError();
 	String GetStatus();
 	virtual void Poll();
-	Task() : listener(NULL) { progress = 0; thProgress = 0; }
+	Task();
 	virtual ~Task();
 protected:
 	int progress;
@@ -35,12 +35,12 @@ protected:
 	TaskListener * listener;
 	pthread_t doWorkThread;
 	pthread_mutex_t taskMutex;
-	pthread_cond_t taskCond;
 
 
 	virtual void before();
 	virtual void after();
 	virtual bool doWork();
+	virtual void doWork_wrapper();
 	TH_ENTRY_POINT static void * doWork_helper(void * ref);
 
 	virtual void notifyProgress(int progress);