Address code review comments/concerns

Azure · Jan 25, 2017 · 1a8cf56 · 1a8cf56
1 parent b09cc9c
commit 1a8cf56
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 31 deletions.
diff --git 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,29 +124,23 @@ struct NODEJS_MODULE_HANDLE_DATA
         module_state = state;
     }
 
-    bool GetStartPending()
-    {
-        nodejs_module::LockGuard<NODEJS_MODULE_HANDLE_DATA> lock_guard(*this);
-        return start_pending;
-    }
-
-    void SetStartPending(bool isPending)
-    {
-        nodejs_module::LockGuard<NODEJS_MODULE_HANDLE_DATA> lock_guard(*this);
-        start_pending = isPending;
-    }
-
     BROKER_HANDLE broker;
-    std::promise<NodeModuleState> create_complete;
     std::string main_path;
     std::string configuration_json;
     v8::Isolate *v8_isolate;
     v8::Persistent<v8::Object> module_object;
     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<NodeModuleState> associated with this
+    *          class will NOT survive a copy or move operation.
+    */
+    std::promise<NodeModuleState> create_complete;
 };
 
 #endif /*NODEJS_COMMON_H*/
diff --git 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<NodeModuleState> 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
         {