C++ destructor is crashing on function pointer

Question

I'm using Visual Studio 2017. I have a class defined thusly:

class Timer
{

friend class TimerFactory;

protected:

explicit Timer(std::function<void()>theCallback, uint theTimer, std::thread::id theThread, bool theImmediate, bool recurring) : 
    myCallback(theCallback), myTimer(theTimer), myThread(theThread), isImmediate(theImmediate), isRecurring(recurring), myWorkComplete(false)
{

}

Timer(const Timer& orig)
{
    myCallback = orig.myCallback;
    myTimer = orig.myTimer;
    myThread = orig.myThread;
    myWorkComplete = orig.myWorkComplete;
    isImmediate = orig.isImmediate;
    isRecurring = orig.isRecurring;
}

public:

~Timer()
{

}

void Start()
{
    Run();
}

private:

std::function<void()>myCallback;
uint myTimer;
std::thread::id myThread;
bool myWorkComplete;
bool isImmediate;
bool isRecurring;

void Run();

};

void Timer::Run()
{
std::chrono::nanoseconds ms(myTimer);

begin: std::this_thread::sleep_for(ms);

if (!(myCallback == nullptr))
{
    myCallback();
}

myWorkComplete = true;

if (isRecurring)
{
    goto begin;
}

if (!isImmediate)
{
    TimerFactory::GetInst()->TimerFired(myThread, this);
}
}

These guys are created like this:

std::function<void()> run_callback = std::bind(&Dispatcher::Run, this);
TimerFactory::GetInst()->CreateTimer(run_callback, MY_DISPATCHER_CLOCK_RATE, true, true);

And

void TimerFactory::CreateTimer(std::function<void()>theCallback, uint  theInterval, bool theImmediate, bool recurring)
{
std::lock_guard<std::mutex> guard(myMutex);
if (myTerminated == true)
{
    return;
}

thread* t = new thread(&TimerFactory::CreateTimerOnThread, theCallback, theInterval, theImmediate, recurring);

if (recurring)
{
    t->detach();
}

else if (theImmediate)
{
    t->join();
}
    else
    {
            myThreads.push_back(t);
    }
}

Followed by:

void TimerFactory::CreateTimerOnThread(std::function<void()>theCallback,  uint theTimer, bool theImmediate, bool recurring)
{
if (theImmediate)
{
    Timer p(theCallback, theTimer, std::this_thread::get_id(), theImmediate, recurring);
    p.Start();
}
else
{
    Timer* p = new (std::nothrow) Timer(theCallback, theTimer, std::this_thread::get_id(), theImmediate, recurring);
    Dispatcher<Timer>::GetInst()->addElement(p);
}
}

The Timer objects that are !isImmediate are the ones that are causing the problem when I pop them off of a list:

template <typename T>
class Dispatcher
{
private:

static Dispatcher<T>* myInstance;
static std::once_flag myOnceFlag;
std::list<T*> myData;
std::mutex myMutex;

Dispatcher()
{
    myInstance = this;
}

public:

static Dispatcher* GetInst()
{
    std::call_once(Dispatcher::myOnceFlag, []() {new Dispatcher(); });
    return Dispatcher::myInstance;
}

virtual void Initialize()
{
    //std::lock_guard<std::mutex> guard(myMutex);

    while (myData.size() > 0)
    {
        myData.pop_back();
    }

    std::function<void()> run_callback = std::bind(&Dispatcher::Run, this);
    TimerFactory::GetInst()->CreateTimer(run_callback, MY_DISPATCHER_CLOCK_RATE, true, true);
}

/* Add an element to my list */
bool addElement(T* theElement)
{
    std::lock_guard<std::mutex> guard(myMutex);

    myData.push_back(theElement);
    return true;
}

/* Clear my list */
void Reset()
{
    std::lock_guard<std::mutex> guard(myMutex);

    while (myData.size() > 0)
    {
        myData.pop_back();
        T* temp = (*myData.begin());
        myData.pop_front();
        temp->Start();
        delete temp; // This causes the exception.

    }
}


virtual void Run()
{
    std::lock_guard<std::mutex> guard(myMutex);

    if (myData.size() > 0)
    {
        T* temp = (*myData.begin());
        myData.pop_front();
        temp->Start();
        delete temp; // This is the line that leads to the exception.
    }
}
};

I'm trying to wrap them in unique_ptrs but when the destructor gets called my application throws the exception:

Exception thrown at 0x0F695DCF (SudokuAPI.dll) in SudokuInterface.exe: 0xC0000005: Access violation reading location 0xDDDDDDDD. occurred

and the call stack is:

SudokuAPI.dll!std::_Func_class<void>::_Tidy() Line 470  C++ Symbols loaded.
SudokuAPI.dll!std::_Func_class<void>::~_Func_class<void>() Line 356 C++ Symbols loaded.
SudokuAPI.dll!std::function<void __cdecl(void)>::~function<void __cdecl(void)>() Line 53    C++ Symbols loaded.
SudokuAPI.dll!Timer::~Timer() Line 35   C++ Symbols loaded.
SudokuAPI.dll!Timer::`scalar deleting destructor'(unsigned int) C++ Non-user code. Symbols loaded.

This exception also occurs when not using unique_ptrs, so I'm stuck. Any thoughts would be greatly appreciated.

There's a lot of code that you aren't showing. The problem appears to be in that code. We are not psychic, we need to see a [Minimal, Complete, and Verifiable example](https://stackoverflow.com/help/mcve) — Caleth, Feb 06 '18 at 15:23
You don’t need to use "std::" every time you can just add "using namespace std;" to your code under the used library’s.http://www.cplusplus.com/forum/beginner/49748/ — dsafewgf2g, Feb 06 '18 at 15:23
@Joel6256 that's a **terrible** [piece of advice](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) — Caleth, Feb 06 '18 at 15:24
@Joel6256: Can, but shouldn't. :P If you must use `using`, import only the stuff you actually use by name. (For instance, you can say `using std::function;`.) — cHao, Feb 06 '18 at 15:24
@cHao Thx for the tip um a C++ beginner to :3 I searched something similar to that and found that question so .... My teacher said never forget “using.namespace std;” :P — dsafewgf2g, Feb 06 '18 at 15:30
std::function run_callback = std::bind(&Dispatcher::Run, this); this pointer is probably get destroied before the Timer as your Timer factory is a singleton. — Jerry, Feb 06 '18 at 15:34
@Joel6256 can't fault you for saying what you were taught; but you'll find that from the comments that it's not uncommon for teachers to give bad advice for long term projects, because it's something that they just don't need to care about. — UKMonkey, Feb 06 '18 at 15:41
Many apologies @Caleth, I've edited my question to add what I think are all the relevant parts. — Brian Clever, Feb 06 '18 at 15:50
You are still not showing the failing code. `TimerFactory::GetInst()->CreateTimer(run_callback, MY_DISPATCHER_CLOCK_RATE, true, true);` is showing the case where `bool theImmediate, bool recurring` are both true. (Which seems in itself an error, as they are checked by other parts like they are mutually exclusive) — Caleth, Feb 06 '18 at 16:05
importantly, when a Timer starts with `isRecurring = true`, the thread it runs on *never ends*. Trying to destroy the `Timer` leads to undefined behaviour, as you have a running function where `this` is an invalid pointer — Caleth, Feb 06 '18 at 16:06
@BrianClever `{ Timer p(theCallback, ...); p.Start(); }` -- A local `p` is created and is destroyed when it goes out of scope, thus calling the destructor of `p`, maybe without you expecting it to be done. Is this your intention? — PaulMcKenzie, Feb 06 '18 at 16:09
It's awfully suspicious to me at first glance a program where there is no synchronization used except for when you're adding items to your `list` container. If I were to guess what the issue is, it is the lack of synchronization, and thus objects being created are being destroyed prematurely, but are attempted to be accessed after destruction. — PaulMcKenzie, Feb 06 '18 at 16:31
Arghhh @Caleth was right. I had a delete of a Timer in a leg of code I had completely forgotten about. What is the protocol, should I delete this post? — Brian Clever, Feb 06 '18 at 16:42
If we are going to help, we need a complete program that we can cut and paste in one piece, not a bunch of fragments that have to be pieced together. We need a description of what you want it to do, and of what it actually does. See [mcve] — Jive Dadson, Feb 06 '18 at 16:44
@BrianClever I'll post that as an answer, you don't need to delete — Caleth, Feb 06 '18 at 16:44

score 0 · Answer 1 · answered Feb 06 '18 at 16:44

0

When a Timer starts with isRecurring = true, the thread it runs on never ends. Trying to destroy the Timer leads to undefined behaviour, as you have a running function where this is an invalid pointer

answered Feb 06 '18 at 16:44

Caleth

52,200
2
44
75

C++ destructor is crashing on function pointer

1 Answers1