From d6048de1006bb23cf6919bd254c3d0dad9a016bb Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 24 Jul 2018 22:32:58 +0200 Subject: [PATCH] json-rpc: Shutdown the JSON-RPC in the context of a DB transaction This needs to be done separately from the rest of the daemon since we can otherwise not make sure that it happens before the DB is freed and we might still need the DN, and be running in a DB transaction, for some destructors to run. --- lightningd/jsonrpc.c | 6 ++---- lightningd/lightningd.c | 8 ++++++++ lightningd/lightningd.h | 7 +++++++ tests/test_rpc.py | 7 +++---- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index cd0b3caf3..a89516554 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -447,7 +447,7 @@ static void parse_request(struct json_connection *jcon, const jsmntok_t tok[]) /* This is a convenient tal parent for duration of command * (which may outlive the conn!). */ - c = tal(jcon->ld, struct command); + c = tal(jcon->ld->rpc_listener, struct command); c->jcon = jcon; c->ld = jcon->ld; c->pending = false; @@ -650,8 +650,7 @@ void setup_jsonrpc(struct lightningd *ld, const char *rpc_filename) err(1, "Listening on '%s'", rpc_filename); log_debug(ld->log, "Listening on '%s'", rpc_filename); - /* Technically this is a leak, but there's only one */ - notleak(io_new_listener(ld, fd, incoming_jcon_connected, ld)); + ld->rpc_listener = io_new_listener(ld->rpc_filename, fd, incoming_jcon_connected, ld); } /** @@ -790,4 +789,3 @@ bool json_tok_wtx(struct wallet_tx * tx, const char * buffer, } return true; } - diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 8ea916ec9..3128fb6a5 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -462,6 +462,14 @@ int main(int argc, char *argv[]) } shutdown_subdaemons(ld); + + /* Clean up the JSON-RPC. This needs to happen in a DB transaction since + * it might actually be touching the DB in some destructors, e.g., + * unreserving UTXOs (see #1737) */ + db_begin_transaction(ld->wallet->db); + tal_free(ld->rpc_listener); + db_commit_transaction(ld->wallet->db); + close(pid_fd); remove(ld->pidfile); diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 1fe8fb3f4..b164eb757 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -78,8 +78,15 @@ struct lightningd { /* Our config dir, and rpc file */ char *config_dir; + + /* Location of the RPC socket. */ char *rpc_filename; + /* The listener for the RPC socket. Can be shut down separately from the + * rest of the daemon to allow a clean shutdown, which frees all pending + * cmds in a DB transaction. */ + struct io_listener *rpc_listener; + /* Configuration file name */ char *config_filename; /* Configuration settings. */ diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 06020d848..ab57dc0a7 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -1,7 +1,6 @@ from fixtures import * # noqa: F401,F403 import os -import pytest import signal import unittest @@ -11,13 +10,13 @@ with open('config.vars') as configfile: DEVELOPER = os.getenv("DEVELOPER", config['DEVELOPER']) == "1" -@pytest.mark.xfail(strict=True) @unittest.skipIf(not DEVELOPER, "needs --dev-disconnect") def test_stop_pending_fundchannel(node_factory, executor): """Stop the daemon while waiting for an accept_channel - This used to crash the node, since we were calling unreserve_utxo while freeing - the daemon, but that needs a DB transaction to be open. + This used to crash the node, since we were calling unreserve_utxo while + freeing the daemon, but that needs a DB transaction to be open. + """ l1 = node_factory.get_node() l2 = node_factory.get_node()