From 6521de81634ac390fd8dcc5a83806bb193d2a6b1 Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Tue, 15 Sep 2015 11:00:05 -0700 Subject: [PATCH 1/4] rework ScriptEngine and worker thread shutdown --- libraries/script-engine/src/ScriptEngine.cpp | 21 +++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 1ef3769970..19e4a8f443 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -107,6 +107,7 @@ ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNam } ScriptEngine::~ScriptEngine() { + qDebug() << "ScriptEngine::~ScriptEngine().... this:" << this << "my thread:" << thread(); // If we're not already in the middle of stopping all scripts, then we should remove ourselves // from the list of running scripts. We don't do this if we're in the process of stopping all scripts // because that method removes scripts from its list as it iterates them @@ -117,20 +118,28 @@ ScriptEngine::~ScriptEngine() { } } +class MyWorkerThread : public QThread { +public: + MyWorkerThread(QObject* parent = nullptr) : QThread(parent) { qDebug() << "MyWorkerThread::MyWorkerThread() this:" << this; } + ~MyWorkerThread() { qDebug() << "MyWorkerThread::~MyWorkerThread() this:" << this; } +}; + void ScriptEngine::runInThread() { - QThread* workerThread = new QThread(this); + QThread* workerThread = new MyWorkerThread(); // thread is owned but ScriptEngine, so if the ScriptEngine is destroyed, the thread will be too. QString scriptEngineName = QString("Script Thread:") + getFilename(); workerThread->setObjectName(scriptEngineName); // when the worker thread is started, call our engine's run.. connect(workerThread, &QThread::started, this, &ScriptEngine::run); - // when the thread is terminated, add both scriptEngine and thread to the deleteLater queue - connect(this, &ScriptEngine::doneRunning, this, &ScriptEngine::deleteLater); + // tell the thread to stop when the script engine is done + connect(this, &ScriptEngine::doneRunning, workerThread, &QThread::quit); + + // when the thread is finished, add thread to the deleteLater queue connect(workerThread, &QThread::finished, workerThread, &QThread::deleteLater); - // tell the thread to stop when the script engine is done - connect(this, &ScriptEngine::destroyed, workerThread, &QThread::quit); + // when the thread is destroyed, add scriptEngine to the deleteLater queue + connect(workerThread, &QThread::finished, this, &ScriptEngine::deleteLater); moveToThread(workerThread); @@ -650,10 +659,12 @@ void ScriptEngine::run() { _isRunning = false; if (_wantSignals) { emit runningStateChanged(); + qDebug() << "ScriptEngine::run().... about to emit doneRunning().... this:" << this << "my thread:" << thread() << "current thread:" << QThread::currentThread(); emit doneRunning(); } _doneRunningThisScript = true; + qDebug() << "ScriptEngine::run().... END OF RUN.... this:" << this << "my thread:" << thread() << "current thread:" << QThread::currentThread(); } // NOTE: This is private because it must be called on the same thread that created the timers, which is why From bdae3e420b21e80dc71c9ebe121b2dbe6717d9c0 Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Tue, 15 Sep 2015 11:24:04 -0700 Subject: [PATCH 2/4] fix crash in AC --- assignment-client/src/Agent.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/assignment-client/src/Agent.cpp b/assignment-client/src/Agent.cpp index ddbe164884..fda226b934 100644 --- a/assignment-client/src/Agent.cpp +++ b/assignment-client/src/Agent.cpp @@ -41,9 +41,6 @@ Agent::Agent(NLPacket& packet) : DEFAULT_WINDOW_STARVE_THRESHOLD, DEFAULT_WINDOW_SECONDS_FOR_DESIRED_CALC_ON_TOO_MANY_STARVES, DEFAULT_WINDOW_SECONDS_FOR_DESIRED_REDUCTION, false)) { - // be the parent of the script engine so it gets moved when we do - _scriptEngine->setParent(this); - DependencyManager::get()->setPacketSender(&_entityEditSender); DependencyManager::set(); @@ -157,6 +154,7 @@ void Agent::run() { qDebug() << "Downloaded script:" << scriptContents; _scriptEngine = new ScriptEngine(scriptContents, _payload); + _scriptEngine->setParent(this); // be the parent of the script engine so it gets moved when we do // setup an Avatar for the script to use ScriptableAvatar scriptedAvatar(_scriptEngine); @@ -255,7 +253,6 @@ void Agent::sendAvatarBillboardPacket() { void Agent::processAgentAvatarAndAudio(float deltaTime) { - qDebug() << "processAgentAvatarAndAudio()"; if (!_scriptEngine->isFinished() && _isAvatar && _avatarData) { const int SCRIPT_AUDIO_BUFFER_SAMPLES = floor(((SCRIPT_DATA_CALLBACK_USECS * AudioConstants::SAMPLE_RATE) From 41caa36038084d53d5be0b28b572c9b2c73307c6 Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Tue, 15 Sep 2015 11:50:10 -0700 Subject: [PATCH 3/4] removed some debug code --- libraries/script-engine/src/ScriptEngine.cpp | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 19e4a8f443..288e60edf0 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -118,14 +118,8 @@ ScriptEngine::~ScriptEngine() { } } -class MyWorkerThread : public QThread { -public: - MyWorkerThread(QObject* parent = nullptr) : QThread(parent) { qDebug() << "MyWorkerThread::MyWorkerThread() this:" << this; } - ~MyWorkerThread() { qDebug() << "MyWorkerThread::~MyWorkerThread() this:" << this; } -}; - void ScriptEngine::runInThread() { - QThread* workerThread = new MyWorkerThread(); // thread is owned but ScriptEngine, so if the ScriptEngine is destroyed, the thread will be too. + QThread* workerThread = new QThread(); // thread is owned but ScriptEngine, so if the ScriptEngine is destroyed, the thread will be too. QString scriptEngineName = QString("Script Thread:") + getFilename(); workerThread->setObjectName(scriptEngineName); @@ -659,12 +653,10 @@ void ScriptEngine::run() { _isRunning = false; if (_wantSignals) { emit runningStateChanged(); - qDebug() << "ScriptEngine::run().... about to emit doneRunning().... this:" << this << "my thread:" << thread() << "current thread:" << QThread::currentThread(); emit doneRunning(); } _doneRunningThisScript = true; - qDebug() << "ScriptEngine::run().... END OF RUN.... this:" << this << "my thread:" << thread() << "current thread:" << QThread::currentThread(); } // NOTE: This is private because it must be called on the same thread that created the timers, which is why From 016a5e5f0a0607ead6a7c4ada9be4c2f600a2240 Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Tue, 15 Sep 2015 11:52:07 -0700 Subject: [PATCH 4/4] cleanup comments --- libraries/script-engine/src/ScriptEngine.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 288e60edf0..a7136edd7b 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -107,7 +107,6 @@ ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNam } ScriptEngine::~ScriptEngine() { - qDebug() << "ScriptEngine::~ScriptEngine().... this:" << this << "my thread:" << thread(); // If we're not already in the middle of stopping all scripts, then we should remove ourselves // from the list of running scripts. We don't do this if we're in the process of stopping all scripts // because that method removes scripts from its list as it iterates them @@ -119,7 +118,7 @@ ScriptEngine::~ScriptEngine() { } void ScriptEngine::runInThread() { - QThread* workerThread = new QThread(); // thread is owned but ScriptEngine, so if the ScriptEngine is destroyed, the thread will be too. + QThread* workerThread = new QThread(); // thread is not owned, so we need to manage the delete QString scriptEngineName = QString("Script Thread:") + getFilename(); workerThread->setObjectName(scriptEngineName); @@ -132,7 +131,7 @@ void ScriptEngine::runInThread() { // when the thread is finished, add thread to the deleteLater queue connect(workerThread, &QThread::finished, workerThread, &QThread::deleteLater); - // when the thread is destroyed, add scriptEngine to the deleteLater queue + // when the thread is finished, add scriptEngine to the deleteLater queue connect(workerThread, &QThread::finished, this, &ScriptEngine::deleteLater); moveToThread(workerThread);