Skip to content

Commit

Permalink
Merge pull request #84 from MafiaHub/81-nodejs-engine-shutdown-crash
Browse files Browse the repository at this point in the history 
scripting: make nodejs reentrant and fix UTs
  • Loading branch information
@zpl-zak
zpl-zak authored Jan 10, 2024
2 parents 2a1782a + 35714c6 commit ef60e0f
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 81 deletions.
140 changes: 94 additions & 46 deletions code/framework/src/scripting/engines/node/engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@ require("vm").runInThisContext(process.argv[1]);

namespace Framework::Scripting::Engines::Node {
EngineError Engine::Init(SDKRegisterCallback cb) {
if (_wasNodeAlreadyInited) {
return EngineError::ENGINE_NODE_INIT_FAILED;
}
// Define the arguments to be passed to the node instance
std::vector<std::string> args = {"mafiahub-server", "--experimental-specifier-resolution=node", "--no-warnings"};
std::vector<std::string> exec_args {};
std::vector<std::string> errors {};

// Initialize the node with the provided arguments
int initCode = node::InitializeNodeWithArgs(&args, &exec_args, &errors);
int initCode = node::InitializeNodeWithArgs(&args, &exec_args, &errors);
if (initCode != 0) {
for (std::string &error : errors) {
Logging::GetLogger(FRAMEWORK_INNER_SCRIPTING)->debug("Failed to initialize node: {}", error);
Expand All @@ -36,11 +39,13 @@ namespace Framework::Scripting::Engines::Node {

// Initialize the platform
_platform = node::MultiIsolatePlatform::Create(4);
v8::V8::InitializePlatform(_platform.get());
v8::V8::InitializePlatform(Engine::_platform.get());
v8::V8::Initialize();

uv_loop_init(&uv_loop);

// Allocate and register the isolate
_isolate = node::NewIsolate(node::CreateArrayBufferAllocator(), uv_default_loop(), _platform.get());
_isolate = node::NewIsolate(node::CreateArrayBufferAllocator(), &uv_loop, _platform.get());

// Allocate our scopes
v8::Locker locker(_isolate);
Expand All @@ -58,13 +63,17 @@ namespace Framework::Scripting::Engines::Node {
// Reset and save the global object template pointer
_globalObjectTemplate.Reset(_isolate, global);

// Ye
Logging::GetLogger(FRAMEWORK_INNER_SCRIPTING)->debug("Node.JS engine initialized!");
_isShuttingDown = false;
_wasNodeAlreadyInited = true;
return EngineError::ENGINE_NONE;
}

EngineError Engine::Shutdown() {
if (!_wasNodeAlreadyInited) {
return EngineError::ENGINE_NODE_SHUTDOWN_FAILED;
}

if (!_platform) {
return EngineError::ENGINE_PLATFORM_NULL;
}
Expand All @@ -75,23 +84,31 @@ namespace Framework::Scripting::Engines::Node {

_isShuttingDown = true;

v8::SealHandleScope seal(_isolate);
// Drain the remaining tasks while there are available ones
do {
uv_run(uv_default_loop(), UV_RUN_DEFAULT);
_platform->DrainTasks(_isolate);
} while (uv_loop_alive(uv_default_loop()));

// Unregister the isolate from the platform
// Release the isolate
bool platform_finished = false;
_platform->AddIsolateFinishedCallback(
_isolate,
[](void *data) {
*static_cast<bool *>(data) = true;
},
&platform_finished);
_platform->UnregisterIsolate(_isolate);

// Wait until the platform has cleaned up all relevant resources.
{
v8::Locker locker(_isolate);
v8::Isolate::Scope isolate_scope(_isolate);
while (!platform_finished) uv_run(&uv_loop, UV_RUN_ONCE);
uv_loop_close(&uv_loop);
}

// Release the isolate
_isolate->Dispose();
_isolate = nullptr;

// Release node and v8 engines
node::FreePlatform(_platform.release());
v8::V8::Dispose();
v8::V8::ShutdownPlatform();
v8::V8::DisposePlatform();
node::FreePlatform(_platform.release());

return EngineError::ENGINE_NONE;
}
Expand All @@ -110,21 +127,29 @@ namespace Framework::Scripting::Engines::Node {
}

// Update the base scripting layer
v8::Locker locker(_isolate);
v8::Isolate::Scope isolateScope(_isolate);
v8::SealHandleScope seal(_isolate);
_platform->DrainTasks(_isolate);
{
v8::Locker locker(_isolate);
v8::Isolate::Scope isolateScope(_isolate);
v8::SealHandleScope seal(_isolate);
_platform->DrainTasks(_isolate);

// Notify the gamemode, if loaded
if (_gamemodeLoaded) {
InvokeEvent(Events[EventIDs::GAMEMODE_UPDATED]);
}
}

// Update the file watcher
if (!_isShuttingDown && Utils::Time::Compare(_nextFileWatchUpdate, Utils::Time::GetTimePoint()) < 0) {
if (_watcher && !_isShuttingDown && Utils::Time::Compare(_nextFileWatchUpdate, Utils::Time::GetTimePoint()) < 0) {
// Process the file changes watcher
_watcher.watch(0);
_watcher->watch(0);
_nextFileWatchUpdate = Utils::Time::Add(Utils::Time::GetTimePoint(), _fileWatchUpdatePeriod);
}

// Notify the gamemode, if loaded
if(_gamemodeLoaded){
InvokeEvent(Events[EventIDs::GAMEMODE_UPDATED]);
if (_shouldReloadWatcher) {
_shouldReloadWatcher = false;
delete _watcher;
PreloadGamemode(_gamemodePath);
}
}
}

Expand Down Expand Up @@ -152,6 +177,8 @@ namespace Framework::Scripting::Engines::Node {
return false;
}

_gamemodePath = mainPath;

auto root = nlohmann::json::parse(packageFileContent);
try {
_gamemodeMetadata.name = root["name"].get<std::string>();
Expand Down Expand Up @@ -216,9 +243,9 @@ namespace Framework::Scripting::Engines::Node {

// Create the execution environment
node::EnvironmentFlags::Flags flags = (node::EnvironmentFlags::Flags)(node::EnvironmentFlags::kOwnsProcessState);
_gamemodeData = node::CreateIsolateData(_isolate, uv_default_loop(), GetPlatform());
_gamemodeData = node::CreateIsolateData(_isolate, &uv_loop, GetPlatform());
std::vector<std::string> argv = {"mafiahub-gamemode"};
_gamemodeEnvironment = node::CreateEnvironment(_gamemodeData, context, argv, argv, flags);
_gamemodeEnvironment = node::CreateEnvironment(_gamemodeData, context, argv, argv, flags);

// Make sure isolate is linked to our node environment
node::IsolateSettings is;
Expand All @@ -240,30 +267,46 @@ namespace Framework::Scripting::Engines::Node {

bool Engine::UnloadGamemode(std::string mainPath) {
// If gamemode is not loaded, don't unload it
if(!_gamemodeLoaded) {
if (!_gamemodeLoaded) {
return false;
}

// Scope the gamemode
v8::Locker locker(_isolate);
v8::Isolate::Scope isolateScope(_isolate);
v8::HandleScope handleScope(_isolate);
v8::Context::Scope scope(GetContext());
// Stop node environment
node::Stop(_gamemodeEnvironment);

// Exit node environment
node::EmitProcessBeforeExit(_gamemodeEnvironment);
node::EmitProcessExit(_gamemodeEnvironment);
{
// Scope the gamemode
v8::Locker locker(_isolate);
v8::Isolate::Scope isolate_scope(_isolate);
{
v8::SealHandleScope seal(_isolate);

// Unregister the SDK
// TODO: fix me
bool more = false;
// Drain the remaining tasks while there are available ones
do {
uv_run(&uv_loop, UV_RUN_DEFAULT);
_platform->DrainTasks(_isolate);

// Stop node environment
node::Stop(_gamemodeEnvironment);
more = uv_loop_alive(&uv_loop);
if (more)
continue;

if (node::EmitProcessBeforeExit(_gamemodeEnvironment).IsNothing())
break;

more = uv_loop_alive(&uv_loop);
} while (more);

// Release memory
node::FreeEnvironment(_gamemodeEnvironment);
node::FreeIsolateData(_gamemodeData);
// Exit node environment
node::EmitProcessExit(_gamemodeEnvironment);
}

// Release memory
node::FreeIsolateData(_gamemodeData);
node::FreeEnvironment(_gamemodeEnvironment);
}

_gamemodeEventHandlers.clear();
_gamemodeLoaded = false;
return true;
}
Expand Down Expand Up @@ -320,13 +363,18 @@ namespace Framework::Scripting::Engines::Node {
return false;
}

_watcher = new cppfs::FileWatcher;

Logging::GetLogger(FRAMEWORK_INNER_SCRIPTING)->debug("Watching '{}' changes", dir.path().c_str());
_watcher.add(dir, cppfs::FileCreated | cppfs::FileRemoved | cppfs::FileModified | cppfs::FileAttrChanged, cppfs::Recursive);
_watcher.addHandler([this, path](cppfs::FileHandle &fh, cppfs::FileEvent ev) {
_watcher->add(dir, cppfs::FileCreated | cppfs::FileRemoved | cppfs::FileModified | cppfs::FileAttrChanged, cppfs::Recursive);
_watcher->addHandler([this, path](cppfs::FileHandle &fh, cppfs::FileEvent ev) {
if (_shouldReloadWatcher)
return;

Logging::GetLogger(FRAMEWORK_INNER_SCRIPTING)->debug("Gamemode is reloaded due to the file changes");
// Close the gamemode first, we'll start with a clean slate
if(this->IsGamemodeLoaded() && UnloadGamemode(path)){
LoadGamemode(path);
_shouldReloadWatcher = true;
}
});

Expand Down
37 changes: 21 additions & 16 deletions code/framework/src/scripting/engines/node/engine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <vector>

#include <node.h>
#include <uv.h>
#include <v8.h>

#include <cppfs/FileHandle.h>
Expand All @@ -24,14 +25,14 @@

#include "../callback.h"
#include "../engine.h"
#include "sdk.h"
#include "v8_helpers/v8_string.h"
#include "v8_helpers/v8_try_catch.h"
#include "sdk.h"

#define V8_RESOURCE_LOCK(engine)\
v8::Locker locker(engine->GetIsolate());\
v8::Isolate::Scope isolateScope(engine->GetIsolate());\
v8::HandleScope handleScope(engine->GetIsolate());\
#define V8_RESOURCE_LOCK(engine) \
v8::Locker locker(engine->GetIsolate()); \
v8::Isolate::Scope isolateScope(engine->GetIsolate()); \
v8::HandleScope handleScope(engine->GetIsolate()); \
v8::Context::Scope contextScope(engine->GetContext());

namespace Framework::Scripting::Engines::Node {
Expand All @@ -48,25 +49,29 @@ namespace Framework::Scripting::Engines::Node {
SDK *_sdk = nullptr;

// Global
cppfs::FileWatcher _watcher;
cppfs::FileWatcher *_watcher;
Utils::Time::TimePoint _nextFileWatchUpdate;
int32_t _fileWatchUpdatePeriod = 1000;

// Global engine
static inline bool _wasNodeAlreadyInited = false;
v8::Isolate *_isolate;
v8::Persistent<v8::ObjectTemplate> _globalObjectTemplate;
std::unique_ptr<node::MultiIsolatePlatform> _platform;
v8::Persistent<v8::Context> _context;
std::string _modName;
std::atomic<bool> _isShuttingDown = false;
uv_loop_t uv_loop;

// Gamemode
std::atomic<bool> _gamemodeLoaded = false;
std::atomic<bool> _gamemodeLoaded = false;
std::string _gamemodePath;
GamemodeMetadata _gamemodeMetadata = {};
v8::Persistent<v8::Script> _gamemodeScript;
node::IsolateData *_gamemodeData = nullptr;
node::Environment *_gamemodeEnvironment = nullptr;

node::IsolateData *_gamemodeData = nullptr;
node::Environment *_gamemodeEnvironment = nullptr;
bool _shouldReloadWatcher = false;

public:
std::map<std::string, std::vector<Callback>> _gamemodeEventHandlers;

Expand All @@ -85,19 +90,19 @@ namespace Framework::Scripting::Engines::Node {
bool WatchGamemodeChanges(std::string);

template <typename... Args>
void InvokeEvent(const std::string name, Args ...args) {
void InvokeEvent(const std::string name, Args... args) {
v8::Locker locker(GetIsolate());
v8::Isolate::Scope isolateScope(GetIsolate());
v8::HandleScope handleScope(GetIsolate());
v8::Context::Scope contextScope(_context.Get(_isolate));

//const auto eventName = Helpers::MakeString(_isolate, name);
// const auto eventName = Helpers::MakeString(_isolate, name);

if (_gamemodeEventHandlers[name].empty()) {
return;
}

constexpr int const arg_count = sizeof...(Args);
constexpr int const arg_count = sizeof...(Args);
v8::Local<v8::Value> v8_args[arg_count ? arg_count : 1] = {v8pp::to_v8(_isolate, std::forward<Args>(args))...};

for (auto it = _gamemodeEventHandlers[name].begin(); it != _gamemodeEventHandlers[name].end(); ++it) {
Expand All @@ -106,9 +111,9 @@ namespace Framework::Scripting::Engines::Node {
it->Get(_isolate)->Call(_context.Get(_isolate), v8::Undefined(_isolate), arg_count, v8_args);

if (tryCatch.HasCaught()) {
auto context = _context.Get(_isolate);
v8::Local<v8::Message> message = tryCatch.Message();
v8::Local<v8::Value> exception = tryCatch.Exception();
auto context = _context.Get(_isolate);
v8::Local<v8::Message> message = tryCatch.Message();
v8::Local<v8::Value> exception = tryCatch.Exception();
v8::MaybeLocal<v8::String> maybeSourceLine = message->GetSourceLine(context);
v8::Maybe<int32_t> line = message->GetLineNumber(context);
v8::ScriptOrigin origin = message->GetScriptOrigin();
Expand Down
2 changes: 1 addition & 1 deletion code/framework/src/scripting/errors.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#pragma once

namespace Framework::Scripting {
enum class EngineError { ENGINE_NONE, ENGINE_NODE_INIT_FAILED, ENGINE_PLATFORM_INIT_FAILED, ENGINE_V8_INIT_FAILED, ENGINE_UV_LOOP_INIT_FAILED, ENGINE_ISOLATE_ALLOCATION_FAILED, ENGINE_ISOLATE_DATA_ALLOCATION_FAILED, ENGINE_PLATFORM_NULL, ENGINE_ISOLATE_NULL };
enum class EngineError { ENGINE_NONE, ENGINE_NODE_INIT_FAILED, ENGINE_PLATFORM_INIT_FAILED, ENGINE_V8_INIT_FAILED, ENGINE_UV_LOOP_INIT_FAILED, ENGINE_ISOLATE_ALLOCATION_FAILED, ENGINE_ISOLATE_DATA_ALLOCATION_FAILED, ENGINE_PLATFORM_NULL, ENGINE_ISOLATE_NULL, ENGINE_NODE_SHUTDOWN_FAILED };

enum class ModuleError { MODULE_NONE, MODULE_ENGINE_NULL, MODULE_ENGINE_INIT_FAILED, MODULE_RESOURCE_MANAGER_NULL };

Expand Down
19 changes: 1 addition & 18 deletions code/tests/modules/scripting_engine_ut.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
MODULE(scripting_engine, {
using namespace Framework::Scripting;

IT("can allocate and deallocate a valid scripting engine instance, then do it again to test re-entry", {
IT("can allocate and deallocate a valid scripting engine instance", {
Module *pEngine = new Module;

// Init the engine and make sure everything went fine
Expand All @@ -26,23 +26,6 @@ MODULE(scripting_engine, {
// Shutdown the engine and make sure everything went down
EQUALS(pEngine->Shutdown(), ModuleError::MODULE_NONE);
EQUALS(reinterpret_cast<Engines::Node::Engine *>(pEngine->GetEngine())->GetIsolate(), nullptr);
EQUALS(reinterpret_cast<Engines::Node::Engine *>(pEngine->GetEngine())->GetPlatform(), nullptr);

delete pEngine;

// Intentionally duplicated to test engine for re-entry
pEngine = new Module;

// Init the engine and make sure everything went fine
EQUALS(pEngine->Init(EngineTypes::ENGINE_NODE, NULL), ModuleError::MODULE_NONE);
NEQUALS(pEngine->GetEngine(), nullptr);
NEQUALS(reinterpret_cast<Engines::Node::Engine *>(pEngine->GetEngine())->GetIsolate(), nullptr);
NEQUALS(reinterpret_cast<Engines::Node::Engine *>(pEngine->GetEngine())->GetPlatform(), nullptr);

// Shutdown the engine and make sure everything went down
EQUALS(pEngine->Shutdown(), ModuleError::MODULE_NONE);
EQUALS(reinterpret_cast<Engines::Node::Engine *>(pEngine->GetEngine())->GetIsolate(), nullptr);
EQUALS(reinterpret_cast<Engines::Node::Engine *>(pEngine->GetEngine())->GetPlatform(), nullptr);

delete pEngine;
});
Expand Down

0 comments on commit ef60e0f

Please sign in to comment.