From 820fdf09e2be812feef4cedc601d84e8ed5da2ec Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 12:42:29 -0700 Subject: [PATCH 01/12] Remove unused ScriptEngine::_wantSignals --- libraries/script-engine/src/ScriptEngine.cpp | 72 ++++++------------- libraries/script-engine/src/ScriptEngine.h | 5 +- libraries/script-engine/src/ScriptEngines.cpp | 2 +- 3 files changed, 24 insertions(+), 55 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index f7ac7894ff..53ffd19869 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -138,10 +138,9 @@ static bool hadUncaughtExceptions(QScriptEngine& engine, const QString& fileName return false; } -ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNameString, bool wantSignals) : +ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNameString) : _scriptContents(scriptContents), _timerFunctionMap(), - _wantSignals(wantSignals), _fileNameString(fileNameString), _arrayBufferClass(new ArrayBufferClass(this)) { @@ -231,15 +230,14 @@ void ScriptEngine::runDebuggable() { return; } stopAllTimers(); // make sure all our timers are stopped if the script is ending - if (_wantSignals) { - emit scriptEnding(); - emit finished(_fileNameString, this); - } + + emit scriptEnding(); + emit finished(_fileNameString, this); _isRunning = false; - if (_wantSignals) { - emit runningStateChanged(); - emit doneRunning(); - } + + emit runningStateChanged(); + emit doneRunning(); + timer->deleteLater(); return; } @@ -249,9 +247,7 @@ void ScriptEngine::runDebuggable() { if (_lastUpdate < now) { float deltaTime = (float)(now - _lastUpdate) / (float)USECS_PER_SECOND; if (!_isFinished) { - if (_wantSignals) { - emit update(deltaTime); - } + emit update(deltaTime); } } _lastUpdate = now; @@ -358,17 +354,13 @@ void ScriptEngine::scriptContentsAvailable(const QUrl& url, const QString& scrip if (QRegularExpression(DEBUG_FLAG).match(scriptContents).hasMatch()) { _debuggable = true; } - if (_wantSignals) { - emit scriptLoaded(url.toString()); - } + emit scriptLoaded(url.toString()); } // FIXME - switch this to the new model of ScriptCache callbacks void ScriptEngine::errorInLoadingScript(const QUrl& url) { qCDebug(scriptengine) << "ERROR Loading file:" << url.toString() << "line:" << __LINE__; - if (_wantSignals) { - emit errorLoadingScript(_fileNameString); // ?? - } + emit errorLoadingScript(_fileNameString); // ?? } // Even though we never pass AnimVariantMap directly to and from javascript, the queued invokeMethod of @@ -785,9 +777,7 @@ QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fi --_evaluatesPending; const auto hadUncaughtException = hadUncaughtExceptions(*this, program.fileName()); - if (_wantSignals) { - emit evaluationFinished(result, hadUncaughtException); - } + emit evaluationFinished(result, hadUncaughtException); return result; } @@ -801,9 +791,7 @@ void ScriptEngine::run() { } _isRunning = true; - if (_wantSignals) { - emit runningStateChanged(); - } + emit runningStateChanged(); QScriptValue result = evaluate(_scriptContents, _fileNameString); @@ -872,9 +860,7 @@ void ScriptEngine::run() { if (_lastUpdate < now) { float deltaTime = (float) (now - _lastUpdate) / (float) USECS_PER_SECOND; if (!_isFinished) { - if (_wantSignals) { - emit update(deltaTime); - } + emit update(deltaTime); } } _lastUpdate = now; @@ -884,9 +870,7 @@ void ScriptEngine::run() { } stopAllTimers(); // make sure all our timers are stopped if the script is ending - if (_wantSignals) { - emit scriptEnding(); - } + emit scriptEnding(); if (entityScriptingInterface->getEntityPacketSender()->serversExist()) { // release the queue of edit entity messages. @@ -904,15 +888,11 @@ void ScriptEngine::run() { } } - if (_wantSignals) { - emit finished(_fileNameString, this); - } + emit finished(_fileNameString, this); _isRunning = false; - if (_wantSignals) { - emit runningStateChanged(); - emit doneRunning(); - } + emit runningStateChanged(); + emit doneRunning(); } // NOTE: This is private because it must be called on the same thread that created the timers, which is why @@ -950,9 +930,7 @@ void ScriptEngine::stop() { return; } _isFinished = true; - if (_wantSignals) { - emit runningStateChanged(); - } + emit runningStateChanged(); } } @@ -1076,9 +1054,7 @@ QUrl ScriptEngine::resolvePath(const QString& include) const { } void ScriptEngine::print(const QString& message) { - if (_wantSignals) { - emit printedMessage(message); - } + emit printedMessage(message); } // If a callback is specified, the included files will be loaded asynchronously and the callback will be called @@ -1214,13 +1190,9 @@ void ScriptEngine::load(const QString& loadFile) { if (_isReloading) { auto scriptCache = DependencyManager::get(); scriptCache->deleteScript(url.toString()); - if (_wantSignals) { - emit reloadScript(url.toString(), false); - } + emit reloadScript(url.toString(), false); } else { - if (_wantSignals) { - emit loadScript(url.toString(), false); - } + emit loadScript(url.toString(), false); } } diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index d37e3eb177..cc23e57fda 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -67,9 +67,7 @@ public: class ScriptEngine : public QScriptEngine, public ScriptUser, public EntitiesScriptEngineProvider { Q_OBJECT public: - ScriptEngine(const QString& scriptContents = NO_SCRIPT, - const QString& fileNameString = QString(""), - bool wantSignals = true); + ScriptEngine(const QString& scriptContents = NO_SCRIPT, const QString& fileNameString = QString("")); ~ScriptEngine(); @@ -191,7 +189,6 @@ protected: bool _isInitialized { false }; QHash _timerFunctionMap; QSet _includedURLs; - bool _wantSignals { true }; QHash _entityScripts; bool _isThreaded { false }; QScriptEngineDebugger* _debugger { nullptr }; diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index 330a94cf0b..f91d7df9e6 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -428,7 +428,7 @@ ScriptEngine* ScriptEngines::loadScript(const QUrl& scriptFilename, bool isUserL return scriptEngine; } - scriptEngine = new ScriptEngine(NO_SCRIPT, "", true); + scriptEngine = new ScriptEngine(NO_SCRIPT, ""); scriptEngine->setUserLoaded(isUserLoaded); connect(scriptEngine, &ScriptEngine::doneRunning, this, [scriptEngine] { scriptEngine->deleteLater(); From 13d602487ff4c57c2b4b2b2da7be6ff2392d14e9 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 12:43:16 -0700 Subject: [PATCH 02/12] Remove guard over atomic in ScriptEngine::stop --- libraries/script-engine/src/ScriptEngine.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 53ffd19869..0ad5087478 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -925,10 +925,6 @@ void ScriptEngine::stopAllTimersForEntityScript(const EntityItemID& entityID) { void ScriptEngine::stop() { if (!_isFinished) { - if (QThread::currentThread() != thread()) { - QMetaObject::invokeMethod(this, "stop"); - return; - } _isFinished = true; emit runningStateChanged(); } From edf82c57ba1104773fdddbc96985fbd9a4a97053 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 12:45:02 -0700 Subject: [PATCH 03/12] Clean scripting thread deletion --- libraries/script-engine/src/ScriptEngine.cpp | 54 +++++++++++--------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 0ad5087478..a27dd7eb10 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -162,16 +162,15 @@ ScriptEngine::~ScriptEngine() { } else { qCWarning(scriptengine) << "Script destroyed after ScriptEngines!"; } - - waitTillDoneRunning(); } void ScriptEngine::disconnectNonEssentialSignals() { disconnect(); - QThread* receiver; + QThread* workerThread; // Ensure the thread should be running, and does exist - if (_isRunning && _isThreaded && (receiver = thread())) { - connect(this, &ScriptEngine::doneRunning, receiver, &QThread::quit); + if (_isRunning && _isThreaded && (workerThread = thread())) { + connect(this, &ScriptEngine::doneRunning, workerThread, &QThread::quit); + connect(workerThread, &QThread::finished, workerThread, &QObject::deleteLater); } } @@ -268,30 +267,33 @@ void ScriptEngine::runInThread() { } _isThreaded = true; - QThread* workerThread = new QThread(); - QString scriptEngineName = QString("Script Thread:") + getFilename(); - workerThread->setObjectName(scriptEngineName); + // The thread interface cannot live on itself, and we want to move this into the thread, so + // the thread cannot have this as a parent. + QThread* workerThread = new QThread(); + workerThread->setObjectName(QString("Script Thread:") + getFilename()); + moveToThread(workerThread); + // NOTE: If you connect any essential signals for proper shutdown or cleanup of // the script engine, make sure to add code to "reconnect" them to the // disconnectNonEssentialSignals() method - - // when the worker thread is started, call our engine's run.. connect(workerThread, &QThread::started, this, &ScriptEngine::run); - - // tell the thread to stop when the script engine is done connect(this, &ScriptEngine::doneRunning, workerThread, &QThread::quit); + connect(workerThread, &QThread::finished, workerThread, &QObject::deleteLater); - moveToThread(workerThread); - - // Starts an event loop, and emits workerThread->started() workerThread->start(); } void ScriptEngine::waitTillDoneRunning() { // If the script never started running or finished running before we got here, we don't need to wait for it auto workerThread = thread(); + if (_isThreaded && workerThread) { + // We should never be waiting (blocking) on our own thread + assert(workerThread != QThread::currentThread()); + + stop(); + QString scriptName = getFilename(); auto startedWaiting = usecTimestampNow(); @@ -301,24 +303,28 @@ void ScriptEngine::waitTillDoneRunning() { // the scripts will likely need to marshall messages across to the main thread, e.g. // if they access Settings or Menu in any of their shutdown code. So: // Process events for the main application thread, allowing invokeMethod calls to pass between threads. - QCoreApplication::processEvents(); // thread-safe :) + QCoreApplication::processEvents(); - // If we've been waiting a second or more, then tell the script engine to stop evaluating + // If the final evaluation takes too long, then tell the script engine to stop evaluating static const auto MAX_SCRIPT_EVALUATION_TIME = USECS_PER_SECOND; + static const auto MAX_SCRIPT_QUITTING_TIME = 0.5 * MAX_SCRIPT_EVALUATION_TIME; auto elapsedUsecs = usecTimestampNow() - startedWaiting; if (elapsedUsecs > MAX_SCRIPT_EVALUATION_TIME) { - qCDebug(scriptengine) << - "Script " << scriptName << " has been running too long [" << elapsedUsecs << " usecs] quitting."; - abortEvaluation(); // to allow the thread to quit - workerThread->quit(); - break; + qCDebug(scriptengine).nospace() << + "Script " << scriptName << " has been running too long [" << elapsedUsecs << "usecs] quitting."; + abortEvaluation(); // break out of current evaluation + + // Wait for the scripting thread to stop running + if (!workerThread->wait(MAX_SCRIPT_QUITTING_TIME)) { + qCWarning(scriptengine).nospace() << + "Script " << scriptName << " has been quitting too long [" << elapsedUsecs << "usecs] terminating."; + workerThread->terminate(); + } } // Avoid a pure busy wait QThread::yieldCurrentThread(); } - - workerThread->deleteLater(); } } From 2cc788f98dce5d0f53e162edffb49a5d3e5ff419 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 12:45:35 -0700 Subject: [PATCH 04/12] Rename ScriptEngine::wait to match std threading --- libraries/script-engine/src/ScriptEngine.cpp | 3 +-- libraries/script-engine/src/ScriptEngine.h | 5 ++++- libraries/script-engine/src/ScriptEngines.cpp | 5 +++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index a27dd7eb10..c52848ad11 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -284,8 +284,7 @@ void ScriptEngine::runInThread() { workerThread->start(); } -void ScriptEngine::waitTillDoneRunning() { - // If the script never started running or finished running before we got here, we don't need to wait for it +void ScriptEngine::wait() { auto workerThread = thread(); if (_isThreaded && workerThread) { diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index cc23e57fda..2c0812210c 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -197,7 +197,10 @@ protected: void init(); QString getFilename() const; - void waitTillDoneRunning(); + + // Stop any evaluating scripts and wait for the scripting thread to finish. + void wait(); + bool evaluatePending() const { return _evaluatesPending > 0; } void timerFired(); void stopAllTimers(); diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index f91d7df9e6..ba18a5019b 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -149,6 +149,7 @@ void ScriptEngines::shutdownScripting() { // NOTE: typically all script engines are running. But there's at least one known exception to this, the // "entities sandbox" which is only used to evaluate entities scripts to test their validity before using // them. We don't need to stop scripts that aren't running. + // TODO: Scripts could be shut down faster if we spread them across a threadpool. if (scriptEngine->isRunning()) { qCDebug(scriptengine) << "about to shutdown script:" << scriptName; @@ -165,12 +166,12 @@ void ScriptEngines::shutdownScripting() { // want any of the scripts final "scriptEnding()" or pending "update()" methods from accessing // any application state after we leave this stopAllScripts() method qCDebug(scriptengine) << "waiting on script:" << scriptName; - scriptEngine->waitTillDoneRunning(); + scriptEngine->wait(); qCDebug(scriptengine) << "done waiting on script:" << scriptName; scriptEngine->deleteLater(); - // If the script is stopped, we can remove it from our set + // Once the script is stopped, we can remove it from our set i.remove(); } } From f40fe88ee7275b5008da01085aec084fb2e72f52 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 13:04:49 -0700 Subject: [PATCH 05/12] Clean up entity script engine deletion --- .../src/EntityTreeRenderer.cpp | 25 ++++++++++--------- .../src/EntityTreeRenderer.h | 2 +- libraries/script-engine/src/ScriptEngines.cpp | 6 +++-- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 814faa8874..1dfe2ab53c 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -77,9 +77,17 @@ EntityTreeRenderer::~EntityTreeRenderer() { int EntityTreeRenderer::_entitiesScriptEngineCount = 0; -void EntityTreeRenderer::setupEntitiesScriptEngine() { - QSharedPointer oldEngine = _entitiesScriptEngine; // save the old engine through this function, so the EntityScriptingInterface doesn't have problems with it. - _entitiesScriptEngine = QSharedPointer(new ScriptEngine(NO_SCRIPT, QString("Entities %1").arg(++_entitiesScriptEngineCount)), &QObject::deleteLater); +void EntityTreeRenderer::resetEntitiesScriptEngine() { + // Keep a ref to oldEngine until newEngine is ready so EntityScriptingInterface has something to use + auto oldEngine = _entitiesScriptEngine; + + auto newEngine = new ScriptEngine(NO_SCRIPT, QString("Entities %1").arg(++_entitiesScriptEngineCount)); + _entitiesScriptEngine = QSharedPointer(newEngine, [](ScriptEngine* engine){ + engine->unloadAllEntityScripts(); + engine->stop(); + engine->deleteLater(); + }); + _scriptingServices->registerScriptEngineWithApplicationServices(_entitiesScriptEngine.data()); _entitiesScriptEngine->runInThread(); DependencyManager::get()->setEntitiesScriptEngine(_entitiesScriptEngine.data()); @@ -87,16 +95,9 @@ void EntityTreeRenderer::setupEntitiesScriptEngine() { void EntityTreeRenderer::clear() { leaveAllEntities(); - if (_entitiesScriptEngine) { - _entitiesScriptEngine->unloadAllEntityScripts(); - _entitiesScriptEngine->stop(); - } if (_wantScripts && !_shuttingDown) { - // NOTE: you can't actually need to delete it here because when we call setupEntitiesScriptEngine it will - // assign a new instance to our shared pointer, which will deref the old instance and ultimately call - // the custom deleter which calls deleteLater - setupEntitiesScriptEngine(); + resetEntitiesScriptEngine(); } auto scene = _viewState->getMain3DScene(); @@ -125,7 +126,7 @@ void EntityTreeRenderer::init() { entityTree->setFBXService(this); if (_wantScripts) { - setupEntitiesScriptEngine(); + resetEntitiesScriptEngine(); } forceRecheckEntities(); // setup our state to force checking our inside/outsideness of entities diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.h b/libraries/entities-renderer/src/EntityTreeRenderer.h index a6fc58e5f1..5c06c5f5cc 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.h +++ b/libraries/entities-renderer/src/EntityTreeRenderer.h @@ -126,7 +126,7 @@ protected: } private: - void setupEntitiesScriptEngine(); + void resetEntitiesScriptEngine(); void addEntityToScene(EntityItemPointer entity); bool findBestZoneAndMaybeContainingEntities(const glm::vec3& avatarPosition, QVector* entitiesContainingAvatar); diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index ba18a5019b..6e7c2bb839 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -158,8 +158,10 @@ void ScriptEngines::shutdownScripting() { // and stop. We can safely short circuit this because we know we're in the "quitting" process scriptEngine->disconnect(this); - // Calling stop on the script engine will set it's internal _isFinished state to true, and result - // in the ScriptEngine gracefully ending it's run() method. + // If this is an entity script, we need to unload any entities + scriptEngine->unloadAllEntityScripts(); + + // Gracefully stop the engine's scripting thread scriptEngine->stop(); // We need to wait for the engine to be done running before we proceed, because we don't From f2f89ca062c50fcb187c211e3b9ce8179946ab85 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 13:39:43 -0700 Subject: [PATCH 06/12] Add logging to ScriptEngine lifetime --- libraries/script-engine/src/ScriptEngine.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index c52848ad11..6ce8c56eb0 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -154,7 +154,7 @@ ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNam } ScriptEngine::~ScriptEngine() { - qCDebug(scriptengine) << "Script Engine shutting down (destructor) for script:" << getFilename(); + qCDebug(scriptengine) << "Script Engine shutting down:" << getFilename(); auto scriptEngines = DependencyManager::get(); if (scriptEngines) { @@ -291,11 +291,10 @@ void ScriptEngine::wait() { // We should never be waiting (blocking) on our own thread assert(workerThread != QThread::currentThread()); + // Engine should be stopped already, but be defensive stop(); - QString scriptName = getFilename(); auto startedWaiting = usecTimestampNow(); - while (workerThread->isRunning()) { // NOTE: This will be called on the main application thread from stopAllScripts. // The application thread will need to continue to process events, because @@ -310,13 +309,13 @@ void ScriptEngine::wait() { auto elapsedUsecs = usecTimestampNow() - startedWaiting; if (elapsedUsecs > MAX_SCRIPT_EVALUATION_TIME) { qCDebug(scriptengine).nospace() << - "Script " << scriptName << " has been running too long [" << elapsedUsecs << "usecs] quitting."; + "Script Engine has been running too long, aborting:" << getFilename(); abortEvaluation(); // break out of current evaluation // Wait for the scripting thread to stop running if (!workerThread->wait(MAX_SCRIPT_QUITTING_TIME)) { qCWarning(scriptengine).nospace() << - "Script " << scriptName << " has been quitting too long [" << elapsedUsecs << "usecs] terminating."; + "Script Engine has been aborting too long, terminating:" << getFilename(); workerThread->terminate(); } } @@ -324,6 +323,8 @@ void ScriptEngine::wait() { // Avoid a pure busy wait QThread::yieldCurrentThread(); } + + qCDebug(scriptengine) << "Script Engine has stopped:" << getFilename(); } } @@ -874,6 +875,8 @@ void ScriptEngine::run() { hadUncaughtExceptions(*this, _fileNameString); } + qCDebug(scriptengine) << "Script Engine stopping:" << getFilename(); + stopAllTimers(); // make sure all our timers are stopped if the script is ending emit scriptEnding(); From 7e82494a663b63d19f43caf2d1e8d1f42ab70fc5 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 13:40:10 -0700 Subject: [PATCH 07/12] Add cap on entities scripting thread stop time --- libraries/entities-renderer/src/EntityTreeRenderer.cpp | 5 +++++ libraries/script-engine/src/ScriptEngine.cpp | 1 - libraries/script-engine/src/ScriptEngine.h | 3 +++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 1dfe2ab53c..c73a7ae6c7 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -83,8 +83,13 @@ void EntityTreeRenderer::resetEntitiesScriptEngine() { auto newEngine = new ScriptEngine(NO_SCRIPT, QString("Entities %1").arg(++_entitiesScriptEngineCount)); _entitiesScriptEngine = QSharedPointer(newEngine, [](ScriptEngine* engine){ + // Gracefully exit engine->unloadAllEntityScripts(); engine->stop(); + + // Disgracefully exit, if necessary + QTimer::singleShot(ScriptEngine::MAX_SCRIPT_EVALUATION_TIME, engine, &ScriptEngine::abort); + engine->deleteLater(); }); diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 6ce8c56eb0..dab921e962 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -304,7 +304,6 @@ void ScriptEngine::wait() { QCoreApplication::processEvents(); // If the final evaluation takes too long, then tell the script engine to stop evaluating - static const auto MAX_SCRIPT_EVALUATION_TIME = USECS_PER_SECOND; static const auto MAX_SCRIPT_QUITTING_TIME = 0.5 * MAX_SCRIPT_EVALUATION_TIME; auto elapsedUsecs = usecTimestampNow() - startedWaiting; if (elapsedUsecs > MAX_SCRIPT_EVALUATION_TIME) { diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 2c0812210c..bb4814eb94 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -67,6 +67,8 @@ public: class ScriptEngine : public QScriptEngine, public ScriptUser, public EntitiesScriptEngineProvider { Q_OBJECT public: + static const auto MAX_SCRIPT_EVALUATION_TIME = USECS_PER_SECOND; + ScriptEngine(const QString& scriptContents = NO_SCRIPT, const QString& fileNameString = QString("")); ~ScriptEngine(); @@ -164,6 +166,7 @@ public: public slots: void callAnimationStateHandler(QScriptValue callback, AnimVariantMap parameters, QStringList names, bool useNames, AnimVariantResultHandler resultHandler); void updateMemoryCost(const qint64&); + void abort() { abortEvaluation(); } signals: void scriptLoaded(const QString& scriptFilename); From 1107882be288f8be23a27892c7087e108b89be0f Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 17:48:14 -0700 Subject: [PATCH 08/12] Throw to stop non-evaluating scripts --- libraries/script-engine/src/ScriptEngine.cpp | 24 +++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index dab921e962..b5077db240 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -304,17 +304,25 @@ void ScriptEngine::wait() { QCoreApplication::processEvents(); // If the final evaluation takes too long, then tell the script engine to stop evaluating - static const auto MAX_SCRIPT_QUITTING_TIME = 0.5 * MAX_SCRIPT_EVALUATION_TIME; auto elapsedUsecs = usecTimestampNow() - startedWaiting; if (elapsedUsecs > MAX_SCRIPT_EVALUATION_TIME) { - qCDebug(scriptengine).nospace() << - "Script Engine has been running too long, aborting:" << getFilename(); - abortEvaluation(); // break out of current evaluation + workerThread->quit(); - // Wait for the scripting thread to stop running - if (!workerThread->wait(MAX_SCRIPT_QUITTING_TIME)) { - qCWarning(scriptengine).nospace() << - "Script Engine has been aborting too long, terminating:" << getFilename(); + if (isEvaluating()) { + qCWarning(scriptengine) << "Script Engine has been running too long, aborting:" << getFilename(); + abortEvaluation(); + } else { + qCWarning(scriptengine) << "Script Engine has been running too long, throwing:" << getFilename(); + auto context = currentContext(); + if (context) { + context->throwError("Timed out during shutdown"); + } + } + + // Wait for the scripting thread to stop running, as + // flooding it with aborts/exceptions will persist it longer + static const auto MAX_SCRIPT_QUITTING_TIME = 0.5 * MSECS_PER_SECOND; + if (workerThread->wait(MAX_SCRIPT_QUITTING_TIME)) { workerThread->terminate(); } } From e1c130d02fe9fcb22e7c47dae143b6f2c3392a2f Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 17:48:34 -0700 Subject: [PATCH 09/12] Timeout long sandbox scripts --- libraries/script-engine/src/ScriptEngine.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index b5077db240..6ed7f7f684 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -1292,8 +1292,23 @@ void ScriptEngine::entityScriptContentAvailable(const EntityItemID& entityID, co setParentURL(scriptOrURL); } + const int SANDBOX_TIMEOUT = 0.25 * MSECS_PER_SECOND; QScriptEngine sandbox; - QScriptValue testConstructor = sandbox.evaluate(program); + sandbox.setProcessEventsInterval(SANDBOX_TIMEOUT); + QScriptValue testConstructor; + { + QTimer timeout; + timeout.setSingleShot(true); + timeout.start(SANDBOX_TIMEOUT); + connect(&timeout, &QTimer::timeout, [&sandbox, SANDBOX_TIMEOUT]{ + auto context = sandbox.currentContext(); + if (context) { + // Guard against infinite loops and non-performant code + context->throwError(QString("Timed out (entity constructors are limited to %1ms)").arg(SANDBOX_TIMEOUT)); + } + }); + testConstructor = sandbox.evaluate(program); + } if (hadUncaughtExceptions(sandbox, program.fileName())) { return; } From 806d06b5529104c6f41f1748381204c1f4c6b9fc Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 12 May 2016 17:49:47 -0700 Subject: [PATCH 10/12] Wait on old entity script engines in threadpool --- .../src/EntityTreeRenderer.cpp | 21 +++++++++++++------ libraries/script-engine/src/ScriptEngine.h | 14 ++++++------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index c73a7ae6c7..7643e446ec 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -83,14 +84,22 @@ void EntityTreeRenderer::resetEntitiesScriptEngine() { auto newEngine = new ScriptEngine(NO_SCRIPT, QString("Entities %1").arg(++_entitiesScriptEngineCount)); _entitiesScriptEngine = QSharedPointer(newEngine, [](ScriptEngine* engine){ - // Gracefully exit + class WaitRunnable : public QRunnable { + public: + WaitRunnable(ScriptEngine* engine) : _engine(engine) {} + virtual void run() override { + _engine->wait(); + _engine->deleteLater(); + } + + private: + ScriptEngine* _engine; + }; + engine->unloadAllEntityScripts(); engine->stop(); - - // Disgracefully exit, if necessary - QTimer::singleShot(ScriptEngine::MAX_SCRIPT_EVALUATION_TIME, engine, &ScriptEngine::abort); - - engine->deleteLater(); + // Wait for the scripting thread from the thread pool to avoid hanging the main thread + QThreadPool::globalInstance()->start(new WaitRunnable(engine)); }); _scriptingServices->registerScriptEngineWithApplicationServices(_entitiesScriptEngine.data()); diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index bb4814eb94..b107e741f4 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -83,6 +83,13 @@ public: /// run the script in the callers thread, exit when stop() is called. void run(); + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // NOTE - this is intended to be a public interface for Agent scripts, and local scripts, but not for EntityScripts + Q_INVOKABLE void stop(); + + // Stop any evaluating scripts and wait for the scripting thread to finish. + void wait(); + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // NOTE - these are NOT intended to be public interfaces available to scripts, the are only Q_INVOKABLE so we can // properly ensure they are only called on the correct thread @@ -138,10 +145,6 @@ public: Q_INVOKABLE void requestGarbageCollection() { collectGarbage(); } - //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// - // NOTE - this is intended to be a public interface for Agent scripts, and local scripts, but not for EntityScripts - Q_INVOKABLE void stop(); - bool isFinished() const { return _isFinished; } // used by Application and ScriptWidget bool isRunning() const { return _isRunning; } // used by ScriptWidget @@ -201,9 +204,6 @@ protected: void init(); QString getFilename() const; - // Stop any evaluating scripts and wait for the scripting thread to finish. - void wait(); - bool evaluatePending() const { return _evaluatesPending > 0; } void timerFired(); void stopAllTimers(); From 2140dc77b3ccbb284c1c018e30d23589ae12378e Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 13 May 2016 16:14:22 -0700 Subject: [PATCH 11/12] Rename wait and unload in best thread --- .../entities-renderer/src/EntityTreeRenderer.cpp | 12 ++++++++---- libraries/script-engine/src/ScriptEngine.cpp | 5 +++-- libraries/script-engine/src/ScriptEngine.h | 6 +----- libraries/script-engine/src/ScriptEngines.cpp | 5 +---- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 7643e446ec..d7f9f1343b 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -88,16 +88,13 @@ void EntityTreeRenderer::resetEntitiesScriptEngine() { public: WaitRunnable(ScriptEngine* engine) : _engine(engine) {} virtual void run() override { - _engine->wait(); + _engine->waitTillDoneRunning(); _engine->deleteLater(); } private: ScriptEngine* _engine; }; - - engine->unloadAllEntityScripts(); - engine->stop(); // Wait for the scripting thread from the thread pool to avoid hanging the main thread QThreadPool::globalInstance()->start(new WaitRunnable(engine)); }); @@ -110,6 +107,13 @@ void EntityTreeRenderer::resetEntitiesScriptEngine() { void EntityTreeRenderer::clear() { leaveAllEntities(); + if (_entitiesScriptEngine) { + // Unload and stop the engine here (instead of in its deleter) to + // avoid marshalling unload signals back to this thread + _entitiesScriptEngine->unloadAllEntityScripts(); + _entitiesScriptEngine->stop(); + } + if (_wantScripts && !_shuttingDown) { resetEntitiesScriptEngine(); } diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 6ed7f7f684..843d714973 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -284,7 +284,7 @@ void ScriptEngine::runInThread() { workerThread->start(); } -void ScriptEngine::wait() { +void ScriptEngine::waitTillDoneRunning() { auto workerThread = thread(); if (_isThreaded && workerThread) { @@ -303,8 +303,9 @@ void ScriptEngine::wait() { // Process events for the main application thread, allowing invokeMethod calls to pass between threads. QCoreApplication::processEvents(); - // If the final evaluation takes too long, then tell the script engine to stop evaluating + // If the final evaluation takes too long, then tell the script engine to stop running auto elapsedUsecs = usecTimestampNow() - startedWaiting; + static const auto MAX_SCRIPT_EVALUATION_TIME = USECS_PER_SECOND; if (elapsedUsecs > MAX_SCRIPT_EVALUATION_TIME) { workerThread->quit(); diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index b107e741f4..789958d57a 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -67,10 +67,7 @@ public: class ScriptEngine : public QScriptEngine, public ScriptUser, public EntitiesScriptEngineProvider { Q_OBJECT public: - static const auto MAX_SCRIPT_EVALUATION_TIME = USECS_PER_SECOND; - ScriptEngine(const QString& scriptContents = NO_SCRIPT, const QString& fileNameString = QString("")); - ~ScriptEngine(); /// run the script in a dedicated thread. This will have the side effect of evalulating @@ -88,7 +85,7 @@ public: Q_INVOKABLE void stop(); // Stop any evaluating scripts and wait for the scripting thread to finish. - void wait(); + void waitTillDoneRunning(); //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // NOTE - these are NOT intended to be public interfaces available to scripts, the are only Q_INVOKABLE so we can @@ -169,7 +166,6 @@ public: public slots: void callAnimationStateHandler(QScriptValue callback, AnimVariantMap parameters, QStringList names, bool useNames, AnimVariantResultHandler resultHandler); void updateMemoryCost(const qint64&); - void abort() { abortEvaluation(); } signals: void scriptLoaded(const QString& scriptFilename); diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index 6e7c2bb839..b31c7ac07b 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -158,9 +158,6 @@ void ScriptEngines::shutdownScripting() { // and stop. We can safely short circuit this because we know we're in the "quitting" process scriptEngine->disconnect(this); - // If this is an entity script, we need to unload any entities - scriptEngine->unloadAllEntityScripts(); - // Gracefully stop the engine's scripting thread scriptEngine->stop(); @@ -168,7 +165,7 @@ void ScriptEngines::shutdownScripting() { // want any of the scripts final "scriptEnding()" or pending "update()" methods from accessing // any application state after we leave this stopAllScripts() method qCDebug(scriptengine) << "waiting on script:" << scriptName; - scriptEngine->wait(); + scriptEngine->waitTillDoneRunning(); qCDebug(scriptengine) << "done waiting on script:" << scriptName; scriptEngine->deleteLater(); From 61fbb7022c1b251e6a8f784a475f38814fccf664 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Mon, 16 May 2016 10:40:11 -0700 Subject: [PATCH 12/12] Mv entities script engine deleter to fn --- .../src/EntityTreeRenderer.cpp | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index d7f9f1343b..bec2fa9b8d 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -78,13 +78,8 @@ EntityTreeRenderer::~EntityTreeRenderer() { int EntityTreeRenderer::_entitiesScriptEngineCount = 0; -void EntityTreeRenderer::resetEntitiesScriptEngine() { - // Keep a ref to oldEngine until newEngine is ready so EntityScriptingInterface has something to use - auto oldEngine = _entitiesScriptEngine; - - auto newEngine = new ScriptEngine(NO_SCRIPT, QString("Entities %1").arg(++_entitiesScriptEngineCount)); - _entitiesScriptEngine = QSharedPointer(newEngine, [](ScriptEngine* engine){ - class WaitRunnable : public QRunnable { +void entitiesScriptEngineDeleter(ScriptEngine* engine) { + class WaitRunnable : public QRunnable { public: WaitRunnable(ScriptEngine* engine) : _engine(engine) {} virtual void run() override { @@ -94,10 +89,18 @@ void EntityTreeRenderer::resetEntitiesScriptEngine() { private: ScriptEngine* _engine; - }; - // Wait for the scripting thread from the thread pool to avoid hanging the main thread - QThreadPool::globalInstance()->start(new WaitRunnable(engine)); - }); + }; + + // Wait for the scripting thread from the thread pool to avoid hanging the main thread + QThreadPool::globalInstance()->start(new WaitRunnable(engine)); +} + +void EntityTreeRenderer::resetEntitiesScriptEngine() { + // Keep a ref to oldEngine until newEngine is ready so EntityScriptingInterface has something to use + auto oldEngine = _entitiesScriptEngine; + + auto newEngine = new ScriptEngine(NO_SCRIPT, QString("Entities %1").arg(++_entitiesScriptEngineCount)); + _entitiesScriptEngine = QSharedPointer(newEngine, entitiesScriptEngineDeleter); _scriptingServices->registerScriptEngineWithApplicationServices(_entitiesScriptEngine.data()); _entitiesScriptEngine->runInThread();