Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use RAII when stopping daemon to cleanup on SIGINT/SIGTERM #1845

Open
wants to merge 1 commit into
base: master
from

Conversation

@devinus
Copy link
Member

devinus commented Mar 20, 2019

This gracefully cleans up resources and frees ports using RAII and adding a signal handler to the daemon.

I expect a discussion around this, but I have yet to experience issues around ports being held on to after the process has died on macOS with this patch which previously has not been the case.

@devinus devinus force-pushed the nano-wallet-company:daemon-handle-signals branch from cbc116c to 053ca6f Mar 20, 2019

@@ -398,7 +398,7 @@ node (node_a), rpc (rpc_a)

nano::ipc::ipc_server::~ipc_server ()
{
node.logger.always_log ("IPC: server stopped");

This comment has been minimized.

Copy link
@wezrule

wezrule Mar 21, 2019

Collaborator

should we check that stop () hasn't already been called in cases like this?:

{
ipc_server server;
server.stop ();
}

Could cause issues I guess.

}
namespace nano_daemon
{
class daemon
{
public:
void run (boost::filesystem::path const &, nano::node_flags const & flags);
daemon (boost::filesystem::path const & data_path, nano::node_flags const & flags);
~daemon ();

This comment has been minimized.

Copy link
@wezrule

wezrule Mar 21, 2019

Collaborator

As you are not using a virtual destructor (which I think is correct), the class should be final so it cannot be subclassed.

nano::node_flags const & flags;
std::shared_ptr<nano::node> node;
std::shared_ptr<nano::rpc> rpc;
std::shared_ptr<nano::ipc::ipc_server> ipc;

This comment has been minimized.

Copy link
@wezrule

wezrule Mar 21, 2019

Collaborator

node, rpc and ipc can all be std::unique_ptr (I know node is unrelated to your change)

daemon.run (data_path, flags);

nano_daemon::daemon daemon (data_path, flags);
on_shutdown = [&](int) {

This comment has been minimized.

Copy link
@wezrule

wezrule Mar 21, 2019

Collaborator

You should just capture what you need i.e [&daemon]

@zhyatt zhyatt added this to CP2 (2019-03-27) in V19 Mar 25, 2019

@zhyatt zhyatt requested review from clemahieu and removed request for argakiig and cryptocode Mar 26, 2019

@clemahieu
Copy link
Collaborator

clemahieu left a comment

Looks good with Wesley's comments.

@@ -407,6 +407,7 @@ void nano::ipc::ipc_server::stop ()
{
transport->stop ();
}
node.logger.always_log ("IPC: server stopped");

This comment has been minimized.

Copy link
@clemahieu

clemahieu Apr 11, 2019

Collaborator

Can we also transports.clear () here so multiple calls to stop won't have an effect?

@zhyatt zhyatt moved this from CP2 (2019-03-27) to During RC in V19 Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.