From 08d43dbd78412bea9965abfb8cad67bfe349f96b Mon Sep 17 00:00:00 2001 From: Jan Date: Fri, 16 Jun 2017 09:41:41 +0200 Subject: [PATCH] refactor handling of V8 contexts and care more about exceptions (#2546) * refactor handling of V8 contexts and care more about exceptions --- arangod/V8Server/V8Context.cpp | 79 +++++++++--- arangod/V8Server/V8Context.h | 15 ++- arangod/V8Server/V8DealerFeature.cpp | 114 +++++++++--------- lib/ApplicationFeatures/V8PlatformFeature.cpp | 1 - lib/V8/v8-utils.cpp | 55 +++++---- 5 files changed, 163 insertions(+), 101 deletions(-) diff --git a/arangod/V8Server/V8Context.cpp b/arangod/V8Server/V8Context.cpp index 441440ee6e..bc684bff6f 100644 --- a/arangod/V8Server/V8Context.cpp +++ b/arangod/V8Server/V8Context.cpp @@ -47,10 +47,36 @@ std::string const GlobalContextMethods::CodeBootstrapCoordinator = std::string const GlobalContextMethods::CodeWarmupExports = "require(\"@arangodb/actions\").warmupExports()"; -V8Context::V8Context(size_t id) - : _id(id), _isolate(nullptr), _locker(nullptr), +V8Context::V8Context(size_t id, v8::Isolate* isolate) + : _id(id), _isolate(isolate), _locker(nullptr), _numExecutions(0), _creationStamp(TRI_microtime()), - _lastGcStamp(0.0), _hasActiveExternals(0) {} + _lastGcStamp(0.0), _hasActiveExternals(false) {} + +void V8Context::lockAndEnter() { + TRI_ASSERT(_isolate != nullptr); + TRI_ASSERT(_locker == nullptr); + _locker = new v8::Locker(_isolate); + _isolate->Enter(); + + TRI_ASSERT(_locker->IsLocked(_isolate)); + TRI_ASSERT(v8::Locker::IsLocked(_isolate)); +} + +void V8Context::unlockAndExit() { + TRI_ASSERT(_locker != nullptr); + TRI_ASSERT(_isolate != nullptr); + + _isolate->Exit(); + delete _locker; + _locker = nullptr; + + TRI_ASSERT(!v8::Locker::IsLocked(_isolate)); +} + +bool V8Context::hasGlobalMethodsQueued() { + MUTEX_LOCKER(mutexLocker, _globalMethodsLock); + return !_globalMethods.empty(); +} double V8Context::age() const { return TRI_microtime() - _creationStamp; @@ -98,24 +124,28 @@ void V8Context::handleGlobalContextMethods() { for (auto& type : copy) { std::string const& func = GlobalContextMethods::code(type); - LOG_TOPIC(DEBUG, arangodb::Logger::FIXME) << "executing global context method '" << func + LOG_TOPIC(DEBUG, arangodb::Logger::V8) << "executing global context method '" << func << "' for context " << _id; TRI_GET_GLOBALS2(_isolate); bool allowUseDatabase = v8g->_allowUseDatabase; v8g->_allowUseDatabase = true; - v8::TryCatch tryCatch; + try { + v8::TryCatch tryCatch; - TRI_ExecuteJavaScriptString( - _isolate, _isolate->GetCurrentContext(), - TRI_V8_STD_STRING2(_isolate, func), - TRI_V8_ASCII_STRING2(_isolate, "global context method"), false); + TRI_ExecuteJavaScriptString( + _isolate, _isolate->GetCurrentContext(), + TRI_V8_STD_STRING2(_isolate, func), + TRI_V8_ASCII_STRING2(_isolate, "global context method"), false); - if (tryCatch.HasCaught()) { - if (tryCatch.CanContinue()) { - TRI_LogV8Exception(_isolate, &tryCatch); + if (tryCatch.HasCaught()) { + if (tryCatch.CanContinue()) { + TRI_LogV8Exception(_isolate, &tryCatch); + } } + } catch (...) { + LOG_TOPIC(WARN, arangodb::Logger::V8) << "caught exception during global context method '" << func << "'"; } v8g->_allowUseDatabase = allowUseDatabase; @@ -125,11 +155,24 @@ void V8Context::handleGlobalContextMethods() { void V8Context::handleCancelationCleanup() { v8::HandleScope scope(_isolate); - LOG_TOPIC(DEBUG, arangodb::Logger::FIXME) << "executing cancelation cleanup context " << _id; + LOG_TOPIC(DEBUG, arangodb::Logger::V8) << "executing cancelation cleanup context " << _id; - TRI_ExecuteJavaScriptString( - _isolate, _isolate->GetCurrentContext(), - TRI_V8_ASCII_STRING2(_isolate, - "require('module')._cleanupCancelation();"), - TRI_V8_ASCII_STRING2(_isolate, "context cleanup method"), false); + try { + TRI_ExecuteJavaScriptString( + _isolate, _isolate->GetCurrentContext(), + TRI_V8_ASCII_STRING2(_isolate, + "require('module')._cleanupCancelation();"), + TRI_V8_ASCII_STRING2(_isolate, "context cleanup method"), false); + } catch (...) { + LOG_TOPIC(WARN, arangodb::Logger::V8) << "caught exception during cancelation cleanup"; + // do not throw from here + } +} + +V8ContextGuard::V8ContextGuard(V8Context* context) : _context(context) { + _context->lockAndEnter(); +} + +V8ContextGuard::~V8ContextGuard() { + _context->unlockAndExit(); } diff --git a/arangod/V8Server/V8Context.h b/arangod/V8Server/V8Context.h index 232ec7b94b..6f23b59785 100644 --- a/arangod/V8Server/V8Context.h +++ b/arangod/V8Server/V8Context.h @@ -110,10 +110,13 @@ class GlobalContextMethods { class V8Context { public: - explicit V8Context(size_t id); + V8Context(size_t id, v8::Isolate* isolate); bool isDefault() const { return _id == 0; } double age() const; + void lockAndEnter(); + void unlockAndExit(); + bool hasGlobalMethodsQueued(); size_t const _id; @@ -133,6 +136,16 @@ class V8Context { void handleGlobalContextMethods(); void handleCancelationCleanup(); }; + +class V8ContextGuard { + public: + explicit V8ContextGuard(V8Context* context); + ~V8ContextGuard(); + + private: + V8Context* _context; +}; + } #endif diff --git a/arangod/V8Server/V8DealerFeature.cpp b/arangod/V8Server/V8DealerFeature.cpp index 28fcf8194e..0a7c7eb651 100644 --- a/arangod/V8Server/V8DealerFeature.cpp +++ b/arangod/V8Server/V8DealerFeature.cpp @@ -279,11 +279,16 @@ V8Context* V8DealerFeature::addContext() { applyContextUpdate(context); - DatabaseFeature* database = - ApplicationServer::getFeature("Database"); + try { + DatabaseFeature* database = + ApplicationServer::getFeature("Database"); - loadJavaScriptFileInContext(database->systemDatabase(), "server/initialize.js", context, nullptr); - return context; + loadJavaScriptFileInContext(database->systemDatabase(), "server/initialize.js", context, nullptr); + return context; + } catch (...) { + delete context; + throw; + } } void V8DealerFeature::unprepare() { @@ -414,17 +419,17 @@ void V8DealerFeature::collectGarbage() { << ", wasDirty: " << wasDirty; bool hasActiveExternals = false; auto isolate = context->_isolate; - TRI_ASSERT(context->_locker == nullptr); - context->_locker = new v8::Locker(isolate); - isolate->Enter(); { + // this guard will lock and enter the isolate + // and automatically exit and unlock it when it runs out of scope + V8ContextGuard contextGuard(context); + v8::HandleScope scope(isolate); auto localContext = v8::Local::New(isolate, context->_context); localContext->Enter(); - { v8::Context::Scope contextScope(localContext); @@ -435,14 +440,9 @@ void V8DealerFeature::collectGarbage() { TRI_RunGarbageCollectionV8(isolate, 1.0); hasActiveExternals = v8g->hasActiveExternals(); } - localContext->Exit(); } - isolate->Exit(); - delete context->_locker; - context->_locker = nullptr; - // update garbage collection statistics context->_hasActiveExternals = hasActiveExternals; context->_numExecutions = 0; @@ -552,10 +552,7 @@ void V8DealerFeature::enterContextInternal(TRI_vocbase_t* vocbase, // turn off memory allocation failures before going into v8 code TRI_DisallowMemoryFailures(); - TRI_ASSERT(context->_locker == nullptr); - context->_locker = new v8::Locker(isolate); - - isolate->Enter(); + context->lockAndEnter(); { v8::HandleScope scope(isolate); auto localContext = v8::Local::New(isolate, context->_context); @@ -819,15 +816,12 @@ void V8DealerFeature::exitContextInternal(V8Context* context) { v8g->_canceled = false; } + // make sure the context will be exited + TRI_DEFER(context->unlockAndExit()); + // check if we need to execute global context methods - bool runGlobal = false; + bool const runGlobal = context->hasGlobalMethodsQueued(); - { - MUTEX_LOCKER(mutexLocker, context->_globalMethodsLock); - runGlobal = !context->_globalMethods.empty(); - } - - // exit the context { v8::HandleScope scope(isolate); @@ -841,7 +835,11 @@ void V8DealerFeature::exitContextInternal(V8Context* context) { TRI_ASSERT(context->_locker->IsLocked(isolate)); TRI_ASSERT(v8::Locker::IsLocked(isolate)); - context->handleGlobalContextMethods(); + try { + context->handleGlobalContextMethods(); + } catch (...) { + // ignore errors here + } } TRI_GET_GLOBALS(); @@ -855,13 +853,6 @@ void V8DealerFeature::exitContextInternal(V8Context* context) { auto localContext = v8::Local::New(isolate, context->_context); localContext->Exit(); } - - isolate->Exit(); - - delete context->_locker; - context->_locker = nullptr; - - TRI_ASSERT(!v8::Locker::IsLocked(isolate)); } void V8DealerFeature::exitContext(V8Context* context) { @@ -891,8 +882,13 @@ void V8DealerFeature::exitContext(V8Context* context) { if (performGarbageCollection && !_freeContexts.empty()) { // only add the context to the dirty list if there is at least one other // free context + + // note that re-adding the context here should not fail as we reserved + // enough room for all contexts during startup _dirtyContexts.emplace_back(context); } else { + // note that re-adding the context here should not fail as we reserved + // enough room for all contexts during startup _freeContexts.emplace_back(context); } @@ -903,6 +899,8 @@ void V8DealerFeature::exitContext(V8Context* context) { CONDITION_LOCKER(guard, _contextCondition); _busyContexts.erase(context); + // note that re-adding the context here should not fail as we reserved + // enough room for all contexts during startup _freeContexts.emplace_back(context); guard.broadcast(); @@ -1097,20 +1095,19 @@ V8Context* V8DealerFeature::buildContext(size_t id) { "V8Platform"); TRI_ASSERT(v8platform != nullptr); + // create isolate v8::Isolate* isolate = v8platform->createIsolate(); - V8Context* context = new V8Context(id); + TRI_ASSERT(isolate != nullptr); - TRI_ASSERT(context->_locker == nullptr); + // pass isolate to a new context + auto context = std::make_unique(id, isolate); - // enter a new isolate - context->_isolate = isolate; - TRI_ASSERT(context->_locker == nullptr); - context->_locker = new v8::Locker(isolate); - context->_isolate->Enter(); + try { + // this guard will lock and enter the isolate + // and automatically exit and unlock it when it runs out of scope + V8ContextGuard contextGuard(context.get()); - // create the context - { - v8::HandleScope handle_scope(isolate); + v8::HandleScope handleScope(isolate); v8::Handle global = v8::ObjectTemplate::New(isolate); @@ -1188,12 +1185,12 @@ V8Context* V8DealerFeature::buildContext(size_t id) { // and return from the context localContext->Exit(); + } catch (...) { + LOG_TOPIC(WARN, Logger::V8) << "caught exception during context initialization"; + v8platform->disposeIsolate(isolate); + throw; } - isolate->Exit(); - delete context->_locker; - context->_locker = nullptr; - // some random delay value to add as an initial garbage collection offset // this avoids collecting all contexts at the very same time double const randomWait = @@ -1206,7 +1203,7 @@ V8Context* V8DealerFeature::buildContext(size_t id) { LOG_TOPIC(TRACE, arangodb::Logger::V8) << "initialized V8 context #" << id; - return context; + return context.release(); } bool V8DealerFeature::loadJavaScriptFileInContext(TRI_vocbase_t* vocbase, @@ -1214,6 +1211,7 @@ bool V8DealerFeature::loadJavaScriptFileInContext(TRI_vocbase_t* vocbase, VPackBuilder* builder) { TRI_ASSERT(vocbase != nullptr); + TRI_ASSERT(context != nullptr); if (_stopping) { return false; @@ -1225,7 +1223,13 @@ bool V8DealerFeature::loadJavaScriptFileInContext(TRI_vocbase_t* vocbase, enterContextInternal(vocbase, context, true); - loadJavaScriptFileInternal(file, context, builder); + try { + loadJavaScriptFileInternal(file, context, builder); + } catch (...) { + LOG_TOPIC(WARN, Logger::V8) << "caught exception while executing JavaScript file '" << file << "' in context #" << context->_id; + exitContextInternal(context); + throw; + } exitContextInternal(context); return true; @@ -1257,7 +1261,7 @@ void V8DealerFeature::loadJavaScriptFileInternal(std::string const& file, V8Cont } } - LOG_TOPIC(TRACE, arangodb::Logger::V8) << "loaded Javascript files for V8 context #" << context->_id; + LOG_TOPIC(TRACE, arangodb::Logger::V8) << "loaded Javascript file '" << file << "' for V8 context #" << context->_id; } void V8DealerFeature::shutdownContext(V8Context* context) { @@ -1265,10 +1269,11 @@ void V8DealerFeature::shutdownContext(V8Context* context) { LOG_TOPIC(TRACE, arangodb::Logger::V8) << "shutting down V8 context #" << context->_id; auto isolate = context->_isolate; - isolate->Enter(); - TRI_ASSERT(context->_locker == nullptr); - context->_locker = new v8::Locker(isolate); { + // this guard will lock and enter the isolate + // and automatically exit and unlock it when it runs out of scope + V8ContextGuard contextGuard(context); + v8::HandleScope scope(isolate); auto localContext = v8::Local::New(isolate, context->_context); @@ -1311,11 +1316,8 @@ void V8DealerFeature::shutdownContext(V8Context* context) { localContext->Exit(); } - context->_context.Reset(); - isolate->Exit(); - delete context->_locker; - context->_locker = nullptr; + context->_context.Reset(); application_features::ApplicationServer::getFeature( "V8Platform")->disposeIsolate(isolate); diff --git a/lib/ApplicationFeatures/V8PlatformFeature.cpp b/lib/ApplicationFeatures/V8PlatformFeature.cpp index 910e5468a6..3e2de552fc 100644 --- a/lib/ApplicationFeatures/V8PlatformFeature.cpp +++ b/lib/ApplicationFeatures/V8PlatformFeature.cpp @@ -189,7 +189,6 @@ v8::Isolate* V8PlatformFeature::createIsolate() { } } - return isolate; } diff --git a/lib/V8/v8-utils.cpp b/lib/V8/v8-utils.cpp index c5728f7012..1c059c79e7 100644 --- a/lib/V8/v8-utils.cpp +++ b/lib/V8/v8-utils.cpp @@ -4447,41 +4447,46 @@ bool TRI_RunGarbageCollectionV8(v8::Isolate* isolate, double availableTime) { idleTimeInMs = 10000; } - TRI_ClearObjectCacheV8(isolate); + try { + TRI_ClearObjectCacheV8(isolate); - double const until = TRI_microtime() + availableTime; + double const until = TRI_microtime() + availableTime; - int totalGcTimeInMs = static_cast(availableTime) * 1000; - int gcAttempts = totalGcTimeInMs / idleTimeInMs; + int totalGcTimeInMs = static_cast(availableTime) * 1000; + int gcAttempts = totalGcTimeInMs / idleTimeInMs; - if (gcAttempts <= 0) { - // minimum is to run the GC once - gcAttempts = 1; - } - - int gcTries = 0; - - while (++gcTries <= gcAttempts) { - if (SingleRunGarbageCollectionV8(isolate, idleTimeInMs)) { - return false; + if (gcAttempts <= 0) { + // minimum is to run the GC once + gcAttempts = 1; } - } - size_t i = 0; - while (TRI_microtime() < until) { - if (++i % 1000 == 0) { - // garbage collection only every x iterations, otherwise we'll use too - // much CPU - if (++gcTries > gcAttempts || - SingleRunGarbageCollectionV8(isolate, idleTimeInMs)) { + int gcTries = 0; + + while (++gcTries <= gcAttempts) { + if (SingleRunGarbageCollectionV8(isolate, idleTimeInMs)) { return false; } } - usleep(1000); - } + size_t i = 0; + while (TRI_microtime() < until) { + if (++i % 1000 == 0) { + // garbage collection only every x iterations, otherwise we'll use too + // much CPU + if (++gcTries > gcAttempts || + SingleRunGarbageCollectionV8(isolate, idleTimeInMs)) { + return false; + } + } - return true; + usleep(1000); + } + + return true; + } catch (...) { + // error caught during garbage collection + return false; + } } ////////////////////////////////////////////////////////////////////////////////