From e9fcd120f8e58d0af1105ec437306b970830a9e1 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Fri, 19 Oct 2018 11:47:48 +1030
Subject: [PATCH] jsonrpc: fix reading of multiple commands.

We occasionaly had a travis hang in test_multirpc, and it's due to a
thinko in the prior patch: if a command completes immediately, it will
do the wake before we go to sleep.  That means we don't digest the
rest of the buffer until the next write.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 lightningd/jsonrpc.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c
index 0f7d260b0..d30562403 100644
--- a/lightningd/jsonrpc.c
+++ b/lightningd/jsonrpc.c
@@ -411,7 +411,8 @@ static void json_command_malformed(struct json_connection *jcon,
 					 JSONRPC2_INVALID_REQUEST, NULL);
 }
 
-static void parse_request(struct json_connection *jcon, const jsmntok_t tok[])
+/* Returns true if command already completed. */
+static bool parse_request(struct json_connection *jcon, const jsmntok_t tok[])
 {
 	const jsmntok_t *method, *id, *params;
 	struct command *c;
@@ -419,7 +420,7 @@ static void parse_request(struct json_connection *jcon, const jsmntok_t tok[])
 	if (tok[0].type != JSMN_OBJECT) {
 		json_command_malformed(jcon, "null",
 				       "Expected {} for json command");
-		return;
+		return true;
 	}
 
 	method = json_get_member(jcon->buffer, tok, "method");
@@ -428,12 +429,12 @@ static void parse_request(struct json_connection *jcon, const jsmntok_t tok[])
 
 	if (!id) {
 		json_command_malformed(jcon, "null", "No id");
-		return;
+		return true;
 	}
 	if (id->type != JSMN_STRING && id->type != JSMN_PRIMITIVE) {
 		json_command_malformed(jcon, "null",
 				       "Expected string/primitive for id");
-		return;
+		return true;
 	}
 
 	/* This is a convenient tal parent for duration of command
@@ -452,13 +453,13 @@ static void parse_request(struct json_connection *jcon, const jsmntok_t tok[])
 	if (!method || !params) {
 		command_fail(c, JSONRPC2_INVALID_REQUEST,
 			     method ? "No params" : "No method");
-		return;
+		return true;
 	}
 
 	if (method->type != JSMN_STRING) {
 		command_fail(c, JSONRPC2_INVALID_REQUEST,
 			     "Expected string for method");
-		return;
+		return true;
 	}
 
 	c->json_cmd = find_cmd(jcon->buffer, method);
@@ -467,14 +468,14 @@ static void parse_request(struct json_connection *jcon, const jsmntok_t tok[])
 			     "Unknown command '%.*s'",
 			     method->end - method->start,
 			     jcon->buffer + method->start);
-		return;
+		return true;
 	}
 	if (c->json_cmd->deprecated && !deprecated_apis) {
 		command_fail(c, JSONRPC2_METHOD_NOT_FOUND,
 			     "Command '%.*s' is deprecated",
 			      method->end - method->start,
 			      jcon->buffer + method->start);
-		return;
+		return true;
 	}
 
 	db_begin_transaction(jcon->ld->wallet->db);
@@ -484,6 +485,8 @@ static void parse_request(struct json_connection *jcon, const jsmntok_t tok[])
 	/* If they didn't complete it, they must call command_still_pending */
 	if (jcon->command == c)
 		assert(c->pending);
+
+	return jcon->command == NULL;
 }
 
 /* Mutual recursion */
@@ -527,7 +530,7 @@ static struct io_plan *read_json(struct io_conn *conn,
 				 struct json_connection *jcon)
 {
 	jsmntok_t *toks;
-	bool valid;
+	bool valid, completed;
 
 	if (jcon->len_read)
 		log_io(jcon->log, LOG_IO_IN, "",
@@ -541,7 +544,7 @@ static struct io_plan *read_json(struct io_conn *conn,
 	toks = json_parse_input(jcon->buffer, jcon->used, &valid);
 	if (!toks) {
 		if (!valid) {
-			log_unusual(jcon->ld->log,
+			log_unusual(jcon->log,
 				    "Invalid token in json input: '%.*s'",
 				    (int)jcon->used, jcon->buffer);
 			json_command_malformed(
@@ -559,17 +562,28 @@ static struct io_plan *read_json(struct io_conn *conn,
 		goto read_more;
 	}
 
-	parse_request(jcon, toks);
+	completed = parse_request(jcon, toks);
 
 	/* Remove first {}. */
 	memmove(jcon->buffer, jcon->buffer + toks[0].end,
 		tal_count(jcon->buffer) - toks[0].end);
 	jcon->used -= toks[0].end;
-	tal_free(toks);
 
-	/* We will get woken by cmd completion. */
+	/* If we haven't completed, wait for cmd completion. */
 	jcon->len_read = 0;
-	return io_wait(conn, conn, read_json, jcon);
+	if (!completed) {
+		tal_free(toks);
+		return io_wait(conn, conn, read_json, jcon);
+	}
+
+	/* If we have more to process, try again.  FIXME: this still gets
+	 * first priority in io_loop, so can starve others.  Hack would be
+	 * a (non-zero) timer, but better would be to have io_loop avoid
+	 * such livelock */
+	if (jcon->used) {
+		tal_free(toks);
+		return io_always(conn, read_json, jcon);
+	}
 
 read_more:
 	tal_free(toks);