From 1f4d974c047ebb95f5c1a7cb9de049acd1b6fa35 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Tue, 24 Jan 2017 09:37:08 -0800 Subject: [PATCH 1/6] Update nodejs_simple_sample JSON Windows config --- samples/nodejs_simple_sample/src/gateway_sample_win.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samples/nodejs_simple_sample/src/gateway_sample_win.json b/samples/nodejs_simple_sample/src/gateway_sample_win.json index fc14e71d..fc0a4fad 100644 --- a/samples/nodejs_simple_sample/src/gateway_sample_win.json +++ b/samples/nodejs_simple_sample/src/gateway_sample_win.json @@ -38,7 +38,7 @@ } }, "args": { - "connection_string": "<>" + "connection_string": "" } }, { From 1854de81b127b5a17dcbdca569420464f487d2a9 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Tue, 24 Jan 2017 09:38:30 -0800 Subject: [PATCH 2/6] Update nodejs_simple_sample JSON Linux config --- .../src/gateway_sample_lin.json | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/samples/nodejs_simple_sample/src/gateway_sample_lin.json b/samples/nodejs_simple_sample/src/gateway_sample_lin.json index c2d37e8b..7e47263b 100644 --- a/samples/nodejs_simple_sample/src/gateway_sample_lin.json +++ b/samples/nodejs_simple_sample/src/gateway_sample_lin.json @@ -1,11 +1,20 @@ { + "loaders": [ + { + "type": "node", + "name": "node", + "configuration": { + "binding.path": "../../../bindings/nodejs/Debug/libnodejs_binding.so" + } + } + ], "modules": [ { "name": "node_printer", "loader": { "name": "node", "entrypoint": { - "main.path": "<>" + "main.path": "../../../samples/nodejs_simple_sample/nodejs_modules/printer.js" } }, "args": null @@ -15,7 +24,7 @@ "loader": { "name": "node", "entrypoint": { - "main.path": "<>" + "main.path": "../../../samples/nodejs_simple_sample/nodejs_modules/sensor.js" } }, "args": null @@ -25,11 +34,11 @@ "loader": { "name": "node", "entrypoint": { - "main.path": "<>" + "main.path": "../../../samples/nodejs_simple_sample/nodejs_modules/iothub_writer.js" } }, "args": { - "connection_string": "<>" + "connection_string": "" } }, { @@ -37,11 +46,11 @@ "loader": { "name": "native", "entrypoint": { - "module.path": "<>" + "module.path": "../../../modules/logger/Debug/liblogger.so" } }, "args": { - "filename": "<>" + "filename": "log.txt" } } ], @@ -59,4 +68,4 @@ "sink": "node_printer" } ] -} \ No newline at end of file +} From 401f2d1ed9cf45710493f9ebc009804789668ebe Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Tue, 24 Jan 2017 15:19:49 -0800 Subject: [PATCH 3/6] Fix build_nodejs.cmd allowing to set env vars --- tools/build_nodejs.cmd | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/build_nodejs.cmd b/tools/build_nodejs.cmd index 4707cc86..0b49b267 100644 --- a/tools/build_nodejs.cmd +++ b/tools/build_nodejs.cmd @@ -47,8 +47,7 @@ copy node\build\Release\lib\*.lib dist\lib popd -rem Export environment variables for where the include/lib files can be found -set "NODE_INCLUDE=%build-root%\dist\inc" -set "NODE_LIB=%build-root%\dist\lib" +rem Set environment variables for where the include/lib files can be found +@endlocal & set NODE_INCLUDE=%build-root%\dist\inc\ & set NODE_LIB=%build-root%\dist\lib\ goto :eof From b6b326028ac02ff4fdcc537004c99c3ec1030555 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Mon, 23 Jan 2017 09:50:08 -0800 Subject: [PATCH 4/6] Serialize nodeJS module create --- bindings/nodejs/inc/nodejs_common.h | 25 ++++++------ bindings/nodejs/src/nodejs.cpp | 60 ++++++++++++++++++++--------- 2 files changed, 55 insertions(+), 30 deletions(-) diff --git a/bindings/nodejs/inc/nodejs_common.h b/bindings/nodejs/inc/nodejs_common.h index 6a1bcaca..fde09cdd 100644 --- a/bindings/nodejs/inc/nodejs_common.h +++ b/bindings/nodejs/inc/nodejs_common.h @@ -4,7 +4,9 @@ #ifndef NODEJS_COMMON_H #define NODEJS_COMMON_H +#include #include +#include #include "v8.h" @@ -18,7 +20,6 @@ typedef void(*PFNMODULE_START)(NODEJS_MODULE_HANDLE_DATA* handle_data); enum class NodeModuleState { error, - initializing, initialized }; @@ -55,6 +56,7 @@ struct NODEJS_MODULE_HANDLE_DATA NODEJS_MODULE_HANDLE_DATA(NODEJS_MODULE_HANDLE_DATA&& rhs) { broker = rhs.broker; + create_complete = std::move(rhs.create_complete); main_path = rhs.main_path; configuration_json = rhs.configuration_json; v8_isolate = rhs.v8_isolate; @@ -140,16 +142,17 @@ struct NODEJS_MODULE_HANDLE_DATA start_pending = isPending; } - BROKER_HANDLE broker; - std::string main_path; - std::string configuration_json; - v8::Isolate *v8_isolate; - v8::Persistent module_object; - size_t module_id; - PFNMODULE_START on_module_start; - NodeModuleState module_state; - bool start_pending; - nodejs_module::Lock object_lock; + BROKER_HANDLE broker; + std::promise create_complete; + std::string main_path; + std::string configuration_json; + v8::Isolate *v8_isolate; + v8::Persistent module_object; + size_t module_id; + PFNMODULE_START on_module_start; + NodeModuleState module_state; + bool start_pending; + nodejs_module::Lock object_lock; }; #endif /*NODEJS_COMMON_H*/ diff --git a/bindings/nodejs/src/nodejs.cpp b/bindings/nodejs/src/nodejs.cpp index 6fdd2bc8..c8702aa6 100644 --- a/bindings/nodejs/src/nodejs.cpp +++ b/bindings/nodejs/src/nodejs.cpp @@ -1,9 +1,15 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -#include +#include #include +#include #include +#include +#include +#include +#include + #include "azure_c_shared_utility/gballoc.h" #ifdef WIN32 @@ -12,11 +18,6 @@ #include #endif -#include -#include -#include -#include - #include "uv.h" #include "v8.h" #include "node.h" @@ -61,6 +62,7 @@ " }" \ "})();" +static void NODEJS_Destroy(MODULE_HANDLE module); static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data); static bool validate_input(BROKER_HANDLE broker, const NODEJS_MODULE_CONFIG* module_config); void call_start_on_module(NODEJS_MODULE_HANDLE_DATA* handle_data); @@ -110,7 +112,31 @@ static MODULE_HANDLE NODEJS_Create(BROKER_HANDLE broker, const void* configurati } else { - handle_data->SetModuleState(NodeModuleState::initializing); + // Wait for the module to become fully initialized + NodeModuleState module_state; + std::future create_gate = handle_data->create_complete.get_future(); + switch (create_gate.wait_for(std::chrono::seconds(10))) + { + case std::future_status::ready: + module_state = create_gate.get(); + break; + case std::future_status::deferred: + case std::future_status::timeout: + default: + module_state = NodeModuleState::error; + break; + } + if (NodeModuleState::initialized != module_state) + { + LogError("Node process failed to start"); + modules_manager->RemoveModule(handle_data->module_id); + NODEJS_Destroy(reinterpret_cast(handle_data)); + handle_data = NULL; + } + else + { + handle_data->SetModuleState(module_state); + } /*Codes_SRS_NODEJS_13_005: [ NodeJS_Create shall return a non-NULL MODULE_HANDLE when successful. ]*/ result = reinterpret_cast(handle_data); @@ -817,7 +843,7 @@ static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data) v8::Isolate* isolate = handle_data->v8_isolate = v8::Isolate::GetCurrent(); if (isolate == nullptr) { - handle_data->SetModuleState(NodeModuleState::error); + handle_data->create_complete.set_value(NodeModuleState::error); LogError("v8 isolate is nullptr"); } else @@ -827,7 +853,7 @@ static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data) auto context = isolate->GetCurrentContext(); if (context.IsEmpty() == true) { - handle_data->SetModuleState(NodeModuleState::error); + handle_data->create_complete.set_value(NodeModuleState::error); LogError("No active v8 context found."); } else @@ -839,7 +865,7 @@ static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data) */ if (create_gateway_host(isolate, context) == false) { - handle_data->SetModuleState(NodeModuleState::error); + handle_data->create_complete.set_value(NodeModuleState::error); LogError("Could not create gateway host in v8 global context"); } else @@ -861,7 +887,7 @@ static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data) auto result = nodejs_module::NodeJSUtils::RunScript(isolate, context, script_str.str()); if (result.IsEmpty() == true) { - handle_data->SetModuleState(NodeModuleState::error); + handle_data->create_complete.set_value(NodeModuleState::error); LogError("registerModule script returned nullptr"); } else @@ -869,7 +895,7 @@ static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data) // this must be boolean and must evaluate to true if (result->IsBoolean() == false) { - handle_data->SetModuleState(NodeModuleState::error); + handle_data->create_complete.set_value(NodeModuleState::error); LogError("Expected registerModule to return boolean but it did not."); } else @@ -877,12 +903,12 @@ static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data) auto bool_result = result->ToBoolean(); if (bool_result->BooleanValue() == false) { - handle_data->SetModuleState(NodeModuleState::error); + handle_data->create_complete.set_value(NodeModuleState::error); LogError("Expected registerModule to return 'true' but it returned 'false'"); } else { - handle_data->SetModuleState(NodeModuleState::initialized); + handle_data->create_complete.set_value(NodeModuleState::initialized); if (handle_data->GetStartPending() == true) { call_start_on_module(handle_data); @@ -1227,11 +1253,7 @@ static void on_start_callback( if (modules_manager->HasModule(module_id) == true) { auto& handle_data = modules_manager->GetModuleFromId(module_id); - if (handle_data.GetModuleState() == NodeModuleState::initializing) - { - handle_data.SetStartPending(true); - } - else if (handle_data.GetModuleState() == NodeModuleState::initialized) + if (handle_data.GetModuleState() == NodeModuleState::initialized) { if (handle_data.module_object.IsEmpty() == false) { From b09cc9c3d373fe80885fbbeeba9d78109840d8a2 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Wed, 25 Jan 2017 10:38:27 -0800 Subject: [PATCH 5/6] Update integration tests --- .../nodejs/tests/nodejs_int/nodejs_int.cpp | 66 ++----------------- 1 file changed, 5 insertions(+), 61 deletions(-) diff --git a/bindings/nodejs/tests/nodejs_int/nodejs_int.cpp b/bindings/nodejs/tests/nodejs_int/nodejs_int.cpp index 43c8eb8e..0f8e574d 100644 --- a/bindings/nodejs/tests/nodejs_int/nodejs_int.cpp +++ b/bindings/nodejs/tests/nodejs_int/nodejs_int.cpp @@ -703,18 +703,9 @@ BEGIN_TEST_SUITE(nodejs_int) auto result = NODEJS_Create(g_broker, &config); ///assert - ASSERT_IS_NOT_NULL(result); - - // wait for 15 seconds for the create to get done - NODEJS_MODULE_HANDLE_DATA* handle_data = reinterpret_cast(result); - wait_for_predicate(15, [handle_data]() { - return handle_data->GetModuleState() != NodeModuleState::initializing; - }); - ASSERT_IS_TRUE(handle_data->GetModuleState() == NodeModuleState::error); - ASSERT_IS_TRUE(handle_data->module_object.IsEmpty() == false); + ASSERT_IS_NULL(result); ///cleanup - NODEJS_Destroy(result); STRING_delete(config.configuration_json); STRING_delete(config.main_path); } @@ -745,18 +736,9 @@ BEGIN_TEST_SUITE(nodejs_int) auto result = NODEJS_Create(g_broker, &config); ///assert - ASSERT_IS_NOT_NULL(result); - - // wait for 15 seconds for the create to get done - NODEJS_MODULE_HANDLE_DATA* handle_data = reinterpret_cast(result); - wait_for_predicate(15, [handle_data]() { - return handle_data->GetModuleState() != NodeModuleState::initializing; - }); - ASSERT_IS_TRUE(handle_data->GetModuleState() == NodeModuleState::error); - ASSERT_IS_TRUE(handle_data->module_object.IsEmpty() == false); + ASSERT_IS_NULL(result); ///cleanup - NODEJS_Destroy(result); STRING_delete(config.configuration_json); STRING_delete(config.main_path); } @@ -788,18 +770,9 @@ BEGIN_TEST_SUITE(nodejs_int) auto result = NODEJS_Create(g_broker, &config); ///assert - ASSERT_IS_NOT_NULL(result); - - // wait for 15 seconds for the create to get done - NODEJS_MODULE_HANDLE_DATA* handle_data = reinterpret_cast(result); - wait_for_predicate(15, [handle_data]() { - return handle_data->GetModuleState() != NodeModuleState::initializing; - }); - ASSERT_IS_TRUE(handle_data->GetModuleState() == NodeModuleState::error); - ASSERT_IS_TRUE(handle_data->module_object.IsEmpty() == false); + ASSERT_IS_NULL(result); ///cleanup - NODEJS_Destroy(result); STRING_delete(config.configuration_json); STRING_delete(config.main_path); } @@ -831,18 +804,9 @@ BEGIN_TEST_SUITE(nodejs_int) auto result = NODEJS_Create(g_broker, &config); ///assert - ASSERT_IS_NOT_NULL(result); - - // wait for 15 seconds for the create to get done - NODEJS_MODULE_HANDLE_DATA* handle_data = reinterpret_cast(result); - wait_for_predicate(15, [handle_data]() { - return handle_data->GetModuleState() != NodeModuleState::initializing; - }); - ASSERT_IS_TRUE(handle_data->GetModuleState() == NodeModuleState::error); - ASSERT_IS_TRUE(handle_data->module_object.IsEmpty() == false); + ASSERT_IS_NULL(result); ///cleanup - NODEJS_Destroy(result); STRING_delete(config.configuration_json); STRING_delete(config.main_path); } @@ -874,17 +838,9 @@ BEGIN_TEST_SUITE(nodejs_int) auto result = NODEJS_Create(g_broker, &config); ///assert - ASSERT_IS_NOT_NULL(result); - - // wait for 15 seconds for the create to get done - NODEJS_MODULE_HANDLE_DATA* handle_data = reinterpret_cast(result); - wait_for_predicate(15, [handle_data]() { - return handle_data->GetModuleState() != NodeModuleState::initializing; - }); - ASSERT_IS_TRUE(handle_data->GetModuleState() == NodeModuleState::error); + ASSERT_IS_NULL(result); ///cleanup - NODEJS_Destroy(result); STRING_delete(config.configuration_json); STRING_delete(config.main_path); } @@ -1210,11 +1166,7 @@ BEGIN_TEST_SUITE(nodejs_int) ///act auto result = NODEJS_Create(g_broker, &config); - // wait for 15 seconds for the create to get done NODEJS_MODULE_HANDLE_DATA* handle_data = reinterpret_cast(result); - wait_for_predicate(15, [handle_data]() { - return handle_data->GetModuleState() != NodeModuleState::initializing; - }); NODEJS_Destroy(result); @@ -1270,11 +1222,7 @@ BEGIN_TEST_SUITE(nodejs_int) NODEJS_Start(result); - // wait for 15 seconds for the create to get done NODEJS_MODULE_HANDLE_DATA* handle_data = reinterpret_cast(result); - wait_for_predicate(15, [handle_data]() { - return handle_data->GetModuleState() != NodeModuleState::initializing; - }); ///assert wait_for_predicate(15, [handle_data]() { @@ -1325,11 +1273,7 @@ BEGIN_TEST_SUITE(nodejs_int) auto result = NODEJS_Create(g_broker, &config); - // wait for 15 seconds for the create to get done NODEJS_MODULE_HANDLE_DATA* handle_data = reinterpret_cast(result); - wait_for_predicate(15, [handle_data]() { - return handle_data->GetModuleState() != NodeModuleState::initializing; - }); ///act From 48304ec509f71cbf356cbe37cc828d57a41926e1 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Wed, 25 Jan 2017 14:35:49 -0800 Subject: [PATCH 6/6] Address code review comments/concerns --- bindings/nodejs/inc/nodejs_common.h | 32 +++++++++-------------------- bindings/nodejs/src/nodejs.cpp | 16 +++++++-------- 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/bindings/nodejs/inc/nodejs_common.h b/bindings/nodejs/inc/nodejs_common.h index fde09cdd..a48f4d2c 100644 --- a/bindings/nodejs/inc/nodejs_common.h +++ b/bindings/nodejs/inc/nodejs_common.h @@ -31,8 +31,7 @@ struct NODEJS_MODULE_HANDLE_DATA on_module_start(nullptr), module_id(0), v8_isolate(nullptr), - module_state(NodeModuleState::error), - start_pending(false) + module_state(NodeModuleState::error) { } @@ -48,22 +47,19 @@ struct NODEJS_MODULE_HANDLE_DATA on_module_start(module_start), module_id(0), v8_isolate(nullptr), - module_state(NodeModuleState::error), - start_pending(false) + module_state(NodeModuleState::error) { } NODEJS_MODULE_HANDLE_DATA(NODEJS_MODULE_HANDLE_DATA&& rhs) { broker = rhs.broker; - create_complete = std::move(rhs.create_complete); main_path = rhs.main_path; configuration_json = rhs.configuration_json; v8_isolate = rhs.v8_isolate; on_module_start = rhs.on_module_start; module_id = rhs.module_id; module_state = rhs.module_state; - start_pending = rhs.start_pending; if (v8_isolate != nullptr && rhs.module_object.IsEmpty() == false) @@ -82,7 +78,6 @@ struct NODEJS_MODULE_HANDLE_DATA on_module_start = rhs.on_module_start; module_id = rhs.module_id; module_state = rhs.module_state; - start_pending = rhs.start_pending; if (v8_isolate != nullptr && rhs.module_object.IsEmpty() == false) @@ -100,7 +95,6 @@ struct NODEJS_MODULE_HANDLE_DATA on_module_start = rhs.on_module_start; this->module_id = module_id; module_state = rhs.module_state; - start_pending = rhs.start_pending; if (v8_isolate != nullptr && rhs.module_object.IsEmpty() == false) { @@ -130,20 +124,7 @@ struct NODEJS_MODULE_HANDLE_DATA module_state = state; } - bool GetStartPending() - { - nodejs_module::LockGuard lock_guard(*this); - return start_pending; - } - - void SetStartPending(bool isPending) - { - nodejs_module::LockGuard lock_guard(*this); - start_pending = isPending; - } - BROKER_HANDLE broker; - std::promise create_complete; std::string main_path; std::string configuration_json; v8::Isolate *v8_isolate; @@ -151,8 +132,15 @@ struct NODEJS_MODULE_HANDLE_DATA size_t module_id; PFNMODULE_START on_module_start; NodeModuleState module_state; - bool start_pending; nodejs_module::Lock object_lock; + + /* + * WARNING: The promise follows the lifespan of the class, NOT + * the lifespan of the handle data. This means the + * std::future associated with this + * class will NOT survive a copy or move operation. + */ + std::promise create_complete; }; #endif /*NODEJS_COMMON_H*/ diff --git a/bindings/nodejs/src/nodejs.cpp b/bindings/nodejs/src/nodejs.cpp index c8702aa6..487b5168 100644 --- a/bindings/nodejs/src/nodejs.cpp +++ b/bindings/nodejs/src/nodejs.cpp @@ -67,6 +67,8 @@ static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data); static bool validate_input(BROKER_HANDLE broker, const NODEJS_MODULE_CONFIG* module_config); void call_start_on_module(NODEJS_MODULE_HANDLE_DATA* handle_data); +static const size_t NODE_LOAD_TIMEOUT_S = 10; + static MODULE_HANDLE NODEJS_Create(BROKER_HANDLE broker, const void* configuration) { MODULE_HANDLE result; @@ -114,16 +116,16 @@ static MODULE_HANDLE NODEJS_Create(BROKER_HANDLE broker, const void* configurati { // Wait for the module to become fully initialized NodeModuleState module_state; - std::future create_gate = handle_data->create_complete.get_future(); - switch (create_gate.wait_for(std::chrono::seconds(10))) + auto create_gate = handle_data->create_complete.get_future(); + switch (create_gate.wait_for(std::chrono::seconds(NODE_LOAD_TIMEOUT_S))) { case std::future_status::ready: - module_state = create_gate.get(); + module_state = create_gate.get(); break; case std::future_status::deferred: case std::future_status::timeout: default: - module_state = NodeModuleState::error; + module_state = NodeModuleState::error; break; } if (NodeModuleState::initialized != module_state) @@ -909,10 +911,7 @@ static void on_module_start(NODEJS_MODULE_HANDLE_DATA* handle_data) else { handle_data->create_complete.set_value(NodeModuleState::initialized); - if (handle_data->GetStartPending() == true) - { - call_start_on_module(handle_data); - } + call_start_on_module(handle_data); } } } @@ -1286,7 +1285,6 @@ static void on_start_callback( { LogError("Module does not have a JS object that needs to be destroyed."); } - handle_data.SetStartPending(false); } else {