CODEWITHSUNDEEP
c++multithreading
KaiserJohaan
KaiserJohaan

Reputation: 9240

Thread wrapper implementation issue

I'm doing a wrapper class around win32 threads and pthreads, in a style similiar to the c++11 thread api. My problem is concerning updating the thread object instance when it's actual thread has exited. Below is a solution I have but I'm not sure if its safe.

/* Thread wrapper class */
    class Thread
    {
    public:
        enum ThreadState
        {
            DETACHED = 0,
            RUNNING
        };

        Thread();
        Thread(void* (*start) (void*), void* arg);
        ~Thread();

        Thread& operator=(Thread& other);
        void Join();
        int32_t SetPriority(int32_t priority);
        void Detach();
        ThreadHandle GetNativeHandle();
        ThreadState GetThreadState();

    private:
        ThreadHandle mHandle;
        ThreadState mState;
        void* (*mFunctionPointer) (void*);
        void* mArg;

        static void* Run(void* arg);
        ThreadHandle _CreateThread(void* (*start) (void*), void* arg);
        void _JoinThread(ThreadHandle& handle);

    };

The second constructor starts a thread. Here's the implementation:

    Thread::Thread(void* (*start) (void*), void* arg)
    {
        mFunctionPointer = start;
        mArg = arg;
        mState = Thread::RUNNING;

        mHandle = _CreateThread(&Run, (void*)this);
        if (!mHandle)
            mState = Thread::DETACHED;
    }

It creates a thread that runs the Run-method and passes along a pointer to this object instance. The reason is once the thread has executed the function it sets the state to DETACHED to signal it is done.

Here is the Run-method

    void* Thread::Run(void* arg)
    {
        Thread* thread = static_cast<Thread*>(arg);

        if (thread && thread->mFunctionPointer && thread->mArg && thread->mState == Thread::RUNNING)
            thread->mFunctionPointer(thread->mArg);

        if (thread && thread->mFunctionPointer && thread->mArg && thread->mState == Thread::RUNNING)
            thread->Detach();

        return NULL;
    }

Also this is the Detach(), which is also called on thread destructor:

    void Thread::Detach()
    {
        mState = Thread::DETACHED;
        mHandle = NULL;
        mArg = NULL;
        mFunctionPointer = NULL;
    }

I feel like this is not safe at all. For example, if the Thread object was constructed on the stack and goes out of scope while its thread is running. The Thread object destructor NULLs its state and data members but the memory location might be overwritten no?

Is there any good way of solving this?

Thanks

Upvotes: 0

Views: 1382

Answers (1)

Torsten Robitzki
Torsten Robitzki

Reputation: 2555

You are doomed, if the Thread instance goes out of scope without the thread being joined before. Make Run() a free or static function and copy all data, that the function need to run into dynamic allocated memory and let Run() free that memory.

There is no need for so much tests in Run(). As your c'tor arguments are already compatible with CreateThread(), just pass the function and the argument to CreateThread and you should be fine.

Kind regards

Torsten

Upvotes: 1

Related Questions