Singularity-Viewer/SingularityViewer
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix for AIStateMachineThread (LLThreadSafeRefCount: deleting non-zero reference)

Browse Source 
This fixes
https://code.google.com/p/singularity-viewer/issues/detail?id=736

The problem was that we need to keep the 'user' derived THREAD_IMPL
alive in the thread, and therefore used an LLPointer<THREAD_IMPL>
(as base class of AIStateMachineThread<THREAD_IMPL>), and therefore
THREAD_IMPL, derived from AIThreadImpl had to be derived from
LLThreadSafeRefCount. However, AIStateMachineThread<THREAD_IMPL> also
needed to be a statemachine of itself and is derived from
AIStateMachineThreadBase derived from AIStateMachine which is ALSO
derived from LLThreadSafeRefCount - that in this case wasn't really
needed. An attempt to deactive it by calling ref() from the constructor
of AIStateMachineThreadBase failed on the fact that LLThreadSafeRefCount
insists that its ref count mRef is exactly zero when it is being
deleted.

The chosen solution is to remove the ref count from AIThreadImpl and use
the LLThreadSafeRefCount base class of AIStateMachineThreadBase. The
result is that not only THREAD_IMPL, but also the state machine object
is kept alive, but that doesn't seem like a problem.

Thus, instead of passing a AIThreadImpl* to
AIStateMachineThreadBase::Thread, we now pass a
AIStateMachineThreadBase* to it to keep the whole of the
AIStateMachineThread<THREAD_IMPL> object alive, which has a THREAD_IMPL
as member now. This member then can be accessed through a virtual
function impl(). Another result of this change is that the 'user' (the
class derived from AIThreadImpl, THREAD_IMPL) now has to deal with the
LLPointer, and use LLPointer<AIStateMachineThread<THREAD_IMPL> >
instead of just AIStateMachineThread<THREAD_IMPL> and also allocate
this object himself. The access from there then changes into a -> to
access the state machine (as opposed to a .) and ->thread_impl() to
access the THREAD_IMPL object (as opposed to a ->).
This commit is contained in:
Aleric Inglewood
2013-04-20 04:28:23 +02:00
committed by Latif Khalifa
parent 70eb45e923
commit 8d028e6948
4 changed files with 43 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
indra/aistatemachine/aistatemachinethread.cpp
View File

@@ -33,12 +33,12 @@
class AIStateMachineThreadBase::Thread : public LLThread {
private:
LLPointer<AIThreadImpl> mImpl;
LLPointer<AIStateMachineThreadBase> mImpl;
bool mNeedCleanup;
public:
Thread(AIThreadImpl* impl) :
Thread(AIStateMachineThreadBase* impl) :
#ifdef LL_DEBUG
LLThread(impl->getName()),
LLThread(impl->impl().getName()),
#else
LLThread("AIStateMachineThreadBase::Thread"),
#endif
@@ -46,7 +46,7 @@ class AIStateMachineThreadBase::Thread : public LLThread {
protected:
/*virtual*/ void run(void)
{
mNeedCleanup = mImpl->thread_done(mImpl->run());
mNeedCleanup = mImpl->impl().thread_done(mImpl->impl().run());
}
/*virtual*/ void terminated(void)
{
@@ -65,7 +65,7 @@ class AIStateMachineThreadBase::Thread : public LLThread {
}
public:
// TODO: Implement a thread pool. For now, just create a new one every time.
static Thread* allocate(AIThreadImpl* impl) { return new Thread(impl); }
static Thread* allocate(AIStateMachineThreadBase* impl) { return new Thread(impl); }
static void completed(Thread* threadp) { delete threadp; }
};
@@ -93,7 +93,7 @@ void AIStateMachineThreadBase::multiplex_impl(state_type run_state)
switch(run_state)
{
case start_thread:
mThread = Thread::allocate(mImpl);
mThread = Thread::allocate(this);
// Set next state.
set_state(wait_stopped);
idle(); // Wait till the thread returns.
@@ -124,10 +124,10 @@ void AIStateMachineThreadBase::abort_impl(void)
{
if (mThread)
{
// If this AIStateMachineThreadBase still exists then the first base class of
// AIStateMachineThread<THREAD_IMPL>, LLPointer<THREAD_IMPL>, also still exists
// and therefore mImpl is valid.
bool need_cleanup = mImpl->state_machine_done(mThread); // Signal the fact that we aborted.
// If this AIStateMachineThreadBase still exists then the AIStateMachineThread<THREAD_IMPL>
// that is derived from it still exists and therefore its member THREAD_IMPL also still exists
// and therefore impl() is valid.
bool need_cleanup = impl().state_machine_done(mThread); // Signal the fact that we aborted.
if (need_cleanup)
{
// This is an unlikely race condition. We have been aborted by our parent,

42
indra/aistatemachine/aistatemachinethread.h
View File

@@ -73,11 +73,11 @@ enum hello_world_state_type {
// The statemachine class (this is almost a template).
class HelloWorld : public AIStateMachine {
private:
AIStateMachineThread<HelloWorldThread> mHelloWorld;
LLPointer<AIStateMachineThread<HelloWorldThread> > mHelloWorld;
bool mErr;
public:
HelloWorld() : mErr(false) { }
HelloWorld() : mHelloWorld(new AIStateMachineThread<HelloWorldThread>), mErr(false) { }
// Print to stderr or stdout?
void init(bool err) { mErr = err; }
@@ -108,7 +108,7 @@ class HelloWorld : public AIStateMachine {
void HelloWorld::initialize_impl(void)
{
mHelloWorld->init(mErr); // Initialize the thread object.
mHelloWorld->thread_impl().init(mErr); // Initialize the thread object.
set_state(HelloWorld_start);
}
@@ -118,14 +118,14 @@ void HelloWorld::multiplex_impl(state_type run_state)
{
case HelloWorld_start:
{
mHelloWorld.run(this, HelloWorld_done); // Run HelloWorldThread and set the state of 'this' to HelloWorld_done when finished.
mHelloWorld->run(this, HelloWorld_done); // Run HelloWorldThread and set the state of 'this' to HelloWorld_done when finished.
idle(HelloWorld_start); // Always go idle after starting a thread!
break;
}
case HelloWorld_done:
{
// We're done. Lets also abort when the thread reported no success.
if (mHelloWorld->successful()) // Read output/result of thread object.
if (mHelloWorld->thread_impl().successful()) // Read output/result of thread object.
finish();
else
abort();
@@ -139,7 +139,7 @@ void HelloWorld::multiplex_impl(state_type run_state)
class AIStateMachineThreadBase;
// Derive from this to implement the code that must run in another thread.
class AIThreadImpl : public LLThreadSafeRefCount {
class AIThreadImpl {
private:
template<typename THREAD_IMPL> friend struct AIStateMachineThread;
typedef AIAccess<AIStateMachineThreadBase*> StateMachineThread_wat;
@@ -178,7 +178,7 @@ class AIStateMachineThreadBase : public AIStateMachine {
static state_type const max_state = wait_stopped + 1;
protected:
AIStateMachineThreadBase(AIThreadImpl* impl) : mImpl(impl) { ref(); /* Never call delete */ }
AIStateMachineThreadBase(void) { }
private:
// Handle initializing the object.
@@ -193,9 +193,11 @@ class AIStateMachineThreadBase : public AIStateMachine {
// Implemenation of state_str for run states.
/*virtual*/ char const* state_str_impl(state_type run_state) const;
// Returns a reference to the implementation code that needs to be run in the thread.
virtual AIThreadImpl& impl(void) = 0;
private:
Thread* mThread; // The thread that the code is run in.
AIThreadImpl* mImpl; // Pointer to the implementation code that needs to be run in the thread.
bool mAbort; // (Inverse of) return value of AIThreadImpl::run(). Only valid in state wait_stopped.
public:
@@ -206,14 +208,22 @@ class AIStateMachineThreadBase : public AIStateMachine {
// The state machine that runs T::run() in a thread.
// THREAD_IMPL Must be derived from AIThreadImpl.
template<typename THREAD_IMPL>
struct AIStateMachineThread : public LLPointer<THREAD_IMPL>, public AIStateMachineThreadBase {
// Constructor.
AIStateMachineThread(void) :
LLPointer<THREAD_IMPL>(new THREAD_IMPL),
AIStateMachineThreadBase(LLPointer<THREAD_IMPL>::get())
{
*AIThreadImpl::StateMachineThread_wat(static_cast<AIThreadImpl*>(LLPointer<THREAD_IMPL>::get())->mStateMachineThread) = this;
}
class AIStateMachineThread : public AIStateMachineThreadBase {
private:
THREAD_IMPL mThreadImpl;
public:
// Constructor.
AIStateMachineThread(void)
{
*AIThreadImpl::StateMachineThread_wat(mThreadImpl.mStateMachineThread) = this;
}
// Accessor.
THREAD_IMPL& thread_impl(void) { return mThreadImpl; }
protected:
/*virtual*/ AIThreadImpl& impl(void) { return mThreadImpl; }
};
#endif

11
indra/newview/llmeshrepository.cpp
View File

@@ -1329,9 +1329,10 @@ void LLMeshUploadThread::preStart()
}
AIMeshUpload::AIMeshUpload(LLMeshUploadThread::instance_list& data, LLVector3& scale, bool upload_textures, bool upload_skin, bool upload_joints, std::string const& upload_url, bool do_upload,
LLHandle<LLWholeModelFeeObserver> const& fee_observer, LLHandle<LLWholeModelUploadObserver> const& upload_observer) : mWholeModelUploadURL(upload_url)
LLHandle<LLWholeModelFeeObserver> const& fee_observer, LLHandle<LLWholeModelUploadObserver> const& upload_observer) :
mMeshUpload(new AIStateMachineThread<LLMeshUploadThread>), mWholeModelUploadURL(upload_url)
{
mMeshUpload->init(data, scale, upload_textures, upload_skin, upload_joints, do_upload, fee_observer, upload_observer);
mMeshUpload->thread_impl().init(data, scale, upload_textures, upload_skin, upload_joints, do_upload, fee_observer, upload_observer);
}
char const* AIMeshUpload::state_str_impl(state_type run_state) const
@@ -1347,7 +1348,7 @@ char const* AIMeshUpload::state_str_impl(state_type run_state) const
void AIMeshUpload::initialize_impl()
{
mMeshUpload->preStart();
mMeshUpload->thread_impl().preStart();
set_state(AIMeshUpload_start);
}
@@ -1356,11 +1357,11 @@ void AIMeshUpload::multiplex_impl(state_type run_state)
switch (run_state)
{
case AIMeshUpload_start:
mMeshUpload.run(this, AIMeshUpload_threadFinished);
mMeshUpload->run(this, AIMeshUpload_threadFinished);
idle(); // Wait till the thread finished.
break;
case AIMeshUpload_threadFinished:
mMeshUpload->postRequest(mWholeModelUploadURL, this);
mMeshUpload->thread_impl().postRequest(mWholeModelUploadURL, this);
idle(); // Wait till the responder finished.
break;
case AIMeshUpload_responderFinished:

2
indra/newview/llmeshrepository.h
View File

@@ -433,7 +433,7 @@ enum AIMeshUpload_state_type {
class AIMeshUpload : public AIStateMachine
{
private:
AIStateMachineThread<LLMeshUploadThread> mMeshUpload;
LLPointer<AIStateMachineThread<LLMeshUploadThread> > mMeshUpload;
std::string mWholeModelUploadURL;
public: