shuts down all contexts on sigint; remove g_sigint_gc

[koonpeng]

This changes the SIGINT handler to only run when no other handlers are installed, it also shuts down all contexts instead of just the default context.

This also fix a bug where a context is not properly shutdown in the sigint handler, with this bug fixed there should no longer be a need to explicitly call process.exit in the handler, we will let node exit gracefully when all work in the event loop are finished.

Discussion: I make the signal handler run only when no other handlers are installed, I did this because maybe an application would want to handle SIGINT differently and not shutdown rclnodejs. But doing this has the side effect that if the app installs a signal handler to clean up it's own resource and it expects rclnodejs to cleanup itself (because that is the behavior when no handlers is installed), rclnodejs's handler would not run.

So given this scenario, I wonder if it is better to

• Run the handler only when it is the only one installed, has the side effect mentioned above.
• Run the handler regardless, apps that wants it's own SIGINT behavior that does not clean things up would have to remove rclnodejs's handler.
• Do not install signal handlers. Users may be confused why ctrl+c does not work by default.

[felixdivo]

See also #721, it is very much related

[minggangw]

Discussion about SIGINT https://github.com/RobotWebTools/rclnodejs/pull/721#issuecomment-726650101

[felixdivo]

I think we should not ignore package-lock.json;). As you said, controversial. But it's AFAIK best practice, and the manual says:

This file is intended to be committed into source repositories, and serves various purposes:

• Describe a single representation of a dependency tree such that teammates, deployments, and continuous integration are guaranteed to install exactly the same dependencies.

• Provide a facility for users to "time-travel" to previous states of node_modules without having to commit the directory itself.

• To facilitate greater visibility of tree changes through readable source control diffs.

• And optimize the installation process by allowing npm to skip repeated metadata resolutions for previously-installed packages.

[koonpeng]

Thanks for the comments. After some thoughts, I changed to default behavior to always cleanup rclnodejs. Reason is because I figured most apps would fall into one of these

• Does not have any long running background tasks, expects ctrl+c to "just work" to gracefully exit the program.
• Has some background tasks, so it installs some signal handlers to clean up it's resources.
• Has some special behavior when it receive SIGINT, it may only want to perform cleanup when some conditions are met.

The first 2 is probably the most common and the new behavior should work for them. Particularly for case 2 because we are no longer forcing node to exit, it should work well with the app's signal handler. For case 3, I added a new function rclnodejs.removeSignalHandlers to allow the app to perform it's own cleanup logic.

[koonpeng]

I think we should not ignore package-lock.json;). As you said, controversial. But it's AFAIK best practice, and the manual says:

This is a complicated topic, the docs recommend committing package-lock.json but there are some arguments which suggests why we should ignore it. See https://dev.to/gajus/stop-using-package-lock-json-or-yarn-lock-3ddi. Since package-lock is not committed in this repo, I assumed that is what rclnodejs prefer. Perhaps @minggangw can help explain if we should/should not ignored it?

[coveralls]

Coverage increased (+0.3%) to 91.515% when pulling cd5e079e3b5d5a47ded52240dcc852ec9619ceac on no-signal-handler into 09766e34ff2e54e846eec284204fbe120b46d998 on develop.

[minggangw]

I haven't met any problem when running npm install without package-lock.json until now.The npm doc said this file should be committed into the repo, but I think if everything runs well without it, we could ignore it now and add it back if something goes wrong in the future.

[felixdivo]

See https://dev.to/gajus/stop-using-package-lock-json-or-yarn-lock-3ddi.

Interesting point. Thank's for sharing. I agree with you now. Delete it and wait what happens. Maybe add the link to that article to the .gitignore file or something like that.

The npm docs should better discuss that...

[koonpeng]

Windows CI is failing sporadically, I wonder if it's because it's using node v12.19.0 which has the segfault bug. Also the cpp build failed, not sure if it's related.

[minggangw]

tep 17/21 : RUN rosdep install --from-paths $ROS2_WS/ros2-linux/share --ignore-src --rosdistro foxy -y --skip-keys "console_bridge fastcdr fastrtps osrf_testing_tools_cpp poco_vendor rmw_connext_cpp rosidl_typesupport_connext_c rosidl_typesupport_connext_cpp rti-connext-dds-5.3.1 tinyxml_vendor tinyxml2_vendor urdfdom urdfdom_headers"
 ---> Running in 00f69051d49d
ERROR: the following packages/stacks could not have their rosdep keys resolved
to system dependencies:
rqt_bag: Cannot locate rosdep definition for [rosbag2_transport_py]

There is an error on travis-ci now, I tried to fix it by #727, but it seems not workable. So, I'd like merge this PR in advance and get back to recheck travis-ci with the next package of https://ci.ros2.org/view/packaging/job/packaging_linux/lastSuccessfulBuild/, thanks!