Skip to content
Permalink
Browse files

Fix AbandonableTask being utterly broken

  • Loading branch information...
LBPHacker committed Mar 22, 2019
1 parent ad71227 commit 5192356b764ed50884e3d039602e5bd868c9630b
Showing with 42 additions and 137 deletions.
  1. +14 −110 src/tasks/AbandonableTask.cpp
  2. +1 −3 src/tasks/AbandonableTask.h
  3. +25 −22 src/tasks/Task.cpp
  4. +2 −2 src/tasks/Task.h
@@ -2,79 +2,18 @@

#include "Platform.h"

#ifdef DEBUG
# 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;
done = false;
thAbandoned = false;
progress = 0;
status = "";
before();
pthread_mutex_init (&taskMutex, NULL);
pthread_create(&doWorkThread, 0, &AbandonableTask::doWork_helper, this);

#ifdef DEBUGTHREADS
std::cerr << "AbandonableTask @ " << this << " created" << std::endl;
#endif
}

TH_ENTRY_POINT void * AbandonableTask::doWork_helper(void * ref)
void AbandonableTask::doWork_wrapper()
{
Task::doWork_helper(ref);
Task::doWork_wrapper();
pthread_cond_signal(&done_cv);

AbandonableTask *task = (AbandonableTask *)ref;
pthread_mutex_lock(&task->taskMutex);
pthread_cond_signal(&task->done_cv);
bool abandoned = task->thAbandoned;
pthread_mutex_unlock(&task->taskMutex);
pthread_mutex_lock(&taskMutex);
bool abandoned = thAbandoned;
pthread_mutex_unlock(&taskMutex);
if (abandoned)
{
pthread_detach(task->doWorkThread);
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;
delete this;
}
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
// take care of the thread if another Poll hasn't already
// (i.e. if done is false despite thDone being true).
// AbandonableTask after unlocking the mutex.
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
// and delete the AbandonableTask later.
// AbandonableTask::doWork_wrapper 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()
{
#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()
AbandonableTask::AbandonableTask() :
thAbandoned(false)
{
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);
}

@@ -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_ */
@@ -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_join(doWorkThread, NULL);
pthread_mutex_destroy(&taskMutex);
}
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 = doWork();
pthread_mutex_lock(&taskMutex);
thSuccess = newSuccess;
thDone = true;
pthread_mutex_unlock(&taskMutex);
}

TH_ENTRY_POINT void *Task::doWork_helper(void *ref)
{
bool newSuccess = ((Task*)ref)->doWork();
pthread_mutex_lock(&((Task*)ref)->taskMutex);
((Task*)ref)->thSuccess = newSuccess;
((Task*)ref)->thDone = true;
pthread_mutex_unlock(&((Task*)ref)->taskMutex);
((Task *)ref)->doWork_wrapper();
return NULL;
}

@@ -17,7 +17,7 @@ class Task {
String GetError();
String GetStatus();
virtual void Poll();
Task() : listener(NULL) { progress = 0; thProgress = 0; }
Task();
virtual ~Task();
protected:
int progress;
@@ -35,12 +35,12 @@ class Task {
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);

0 comments on commit 5192356

Please sign in to comment.
You can’t perform that action at this time.