-
Notifications
You must be signed in to change notification settings - Fork 258
Conversation
Hi @zafields, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
Addresses issue #96 by making the node binding create synchronous, and updates the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need changes to the integration tests as well. Will wait to see those. Also, it'll be good to squash the commits once everything is addressed.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, but did we not decide not to do this in the move constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of the life of any individual data structure used to contain the handle data, the collection of handle data represents a single object. So I believe the future (along with all other data) should be moved if possible to reflect the intent of what is happening.
Unfortunately, we are only able to preserve this data in the MOVE constructor and not in the copy constructors. I see how this is more confusing than not bring them along all together, but I must admit I don't like the way it feels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave a comment to this effect and forget about it. :D
handle_data->SetModuleState(NodeModuleState::initializing); | ||
// Wait for the module to become fully initialized | ||
NodeModuleState module_state; | ||
std::future<NodeModuleState> create_gate = handle_data->create_complete.get_future(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use auto
instead of spelling out the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
// 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))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define a constant for the timeout period of 10
seconds? In fact the constant could perhaps be a std::chrono
instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gave the number a name for now.
{ | ||
case std::future_status::ready: | ||
module_state = create_gate.get(); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: break
indentation should align with line 121. Here and on line 127.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed (overly helpful VS caused this).
size_t module_id; | ||
PFNMODULE_START on_module_start; | ||
NodeModuleState module_state; | ||
bool start_pending; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind having this flag start_pending
was to handle the case where a call to NODEJS_Start
occurs before the module's state has transitioned to initialized
from initializing
. Now that NODEJS_Create
is a blocking call and we no longer have an initializing
state, we may not need this flag anymore. Consider removing this field and the associated logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed with test passing
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\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
1a8cf56
to
48304ec
Compare
No description provided.