Overview
Features
Download
Documentation
Community
Add-Ons & Services

Error in Win32 ThreadImpl::start(Runnable&)

Please post support and help requests here.

Error in Win32 ThreadImpl::start(Runnable&)

Postby martinrohn » 01 Feb 2013, 14:45

There is a bug in Win32 ThreadImpl::startImpl(), see below:

Code: Select all
void ThreadImpl::startImpl(Runnable& target)
{
        .... 
   _pRunnableTarget = ⌖
        ....

}

Here, pRunnableTarget should contain a deep copy of the target argument, not just a pointer to it.

Since pRunnableTarget is used at the moment the thread is spawned (asynchronously to this call).
And if the Runnable object passed to startImpl has been destructed meanwhile, the process is likely to crash.

I have created a simple example that crashes on my machine every time (Windows 7):
Code: Select all
#include "Poco/Thread.h"
#include "Poco/Event.h"

class Runner
{

public:
    void Start();
    void Stop();
    void ThreadProc();
private:   
    void DoSomeWork();

private:

    Poco::Thread m_Thread;
    Poco::Event m_StopHandle;
};


int main()
{
  Runner r;
  r.start();
  Poco::Thread::sleep(5000);
  return 0;
}


void Runner::Start()
{
    RunnableAdapter<Runner> ra(*this, &Runner::ThreadProc);
    m_Thread.start(ra);
}

void Runner::Stop()
{
    m_StopHandle.set();
}

void Runner::DoSomeWork()
{
    /*...*/
}

void Runner::ThreadProc()
{
    while (true)
    {
            bool result = m_StopHandle.tryWait(500);
            if (result)
            {
                //stop request -> terminate
                return;
            }
   
            DoSomeWork();
    }
}
martinrohn
 
Posts: 2
Joined: 01 Feb 2013, 14:22

Re: Error in Win32 ThreadImpl::start(Runnable&)

Postby alex » 01 Feb 2013, 15:15

What is the purpose of Runner::Stop()? Why would one want to call sleep() instead?
alex
 
Posts: 1105
Joined: 11 Jul 2006, 16:27
Location: United_States

Re: Error in Win32 ThreadImpl::start(Runnable&)

Postby guenter » 01 Feb 2013, 15:29

You are responsible to make sure your Runnable object remains alive during the entire lifetime of the thread. Passing a local Runnable to Thread::start() and then returning from the function is a recipe for disaster. This is not Windows-specific and happens on all platforms.
guenter
 
Posts: 1112
Joined: 11 Jul 2006, 16:27
Location: Austria

Re: Error in Win32 ThreadImpl::start(Runnable&)

Postby alex » 01 Feb 2013, 15:58

guenter wrote:Passing a local Runnable to Thread::start() and then returning from the function is a recipe for disaster.

Of course, I guess that's why he proposed to copy it, apparently thinking it would solve the problem. But, even if Runnable was copied (or, more appropriately, ra turned into a member of Runner), the ThreadProc could still be running passed the sleep() point (and destruction of Runner). Bottom line, if you launch a thread, make sure (1) all objects accessed by it are valid during its lifetime, (2) there is a clan exit strategy and (3) be sure it's completed (join with it) before your program terminates.
alex
 
Posts: 1105
Joined: 11 Jul 2006, 16:27
Location: United_States

Re: Error in Win32 ThreadImpl::start(Runnable&)

Postby martinrohn » 01 Feb 2013, 17:17

guenter wrote:You are responsible to make sure your Runnable object remains alive during the entire lifetime of the thread. Passing a local Runnable to Thread::start() and then returning from the function is a recipe for disaster. This is not Windows-specific and happens on all platforms.


Is this documented somewhere? I did not see any mention about this when reading Poco online documentation.
I also do not think that it is a good design decision - when a client instantiates a Thread, he also must preserve a Runnable object for its whole lifetime without a reason (at least for me).

Thanks,
Martin
martinrohn
 
Posts: 2
Joined: 01 Feb 2013, 14:22

Re: Error in Win32 ThreadImpl::start(Runnable&)

Postby alex » 02 Feb 2013, 06:46

guenter wrote:You are responsible to make sure your Runnable object remains alive during the entire lifetime of the thread. Passing a local Runnable to Thread::start() and then returning from the function is a recipe for disaster. This is not Windows-specific and happens on all platforms.

martinrohn wrote:Is this documented somewhere? I did not see any mention about this when reading Poco online documentation.

It is not explicitly documented but, as already stated, the peril of spawning a thread and passing in a reference to ephemeral storage is not POCO or Windows specific. It's just something you shouldn't do. It's easy to notice that Runnable is passed as a (non-const) reference, which implies that it will be used in there and should remain alive.
martinrohn wrote:I also do not think that it is a good design decision - when a client instantiates a Thread, he also must preserve a Runnable object for its whole lifetime without a reason (at least for me).

I can think of scenarios where this may be useful (e.g. if one wants to "chain" threads in a "future.then()" fashion). The way it could be done without breaking existing semantics is by introducing virtual Runnable::clone() and adding a default bool param to Thread::start() indicating whether Thread should own the Runnable or not. Not sure if it's worth it, though. If you need it that bad, go ahead do the change and submit github pull request, we'll look into it.

In regards to the problem in the original example, the solution is simple - make ra a member of Runner, call Runner::Stop() in ~Runner() and wait for the worker to complete. You may also consider using Activity.
alex
 
Posts: 1105
Joined: 11 Jul 2006, 16:27
Location: United_States


Return to Support

Who is online

Users browsing this forum: No registered users and 3 guests