Merge bitcoin#18335: bitcoin-cli: print useful error if bitcoind rpc work queue exceeded

MarcoFalke · MarcoFalke · commit 3530d5d2d851 · 2021-03-19T20:52:16.000+01:00
8dd5946 add functional test (Larry Ruane) b5a80fa util: Handle HTTP_SERVICE_UNAVAILABLE in bitcoin-cli (Hennadii Stepanov) Pull request description: If `bitcoind` is processing 16 RPC requests, attempting to submit another request using `bitcoin-cli` produces this less-than-helpful error message: `error: couldn't parse reply from server`. This PR changes the error to: `error: server response: Work queue depth exceeded`. ACKs for top commit: fjahr: tACK 8dd5946 luke-jr: utACK 8dd5946 (no changes since previous utACK) hebasto: re-ACK 8dd5946, only suggested changes since my [previous](bitcoin#18335 (review)) review. darosior: ACK 8dd5946 Tree-SHA512: 33e25f6ff05d9b56fae2bdb68b132557bb8e995f5438ac4fbbc53c304c5152a98aa43c43600c31d8a6a2830cbd48bf8ec7d89dce50190b29ec00a43830126913
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
@@ -708,6 +708,8 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
         } else {
             throw std::runtime_error("Authorization failed: Incorrect rpcuser or rpcpassword");
         }
+    } else if (response.status == HTTP_SERVICE_UNAVAILABLE) {
+        throw std::runtime_error(strprintf("Server response: %s", response.body));
     } else if (response.status >= 400 && response.status != HTTP_BAD_REQUEST && response.status != HTTP_NOT_FOUND && response.status != HTTP_INTERNAL_SERVER_ERROR)
         throw std::runtime_error(strprintf("server returned HTTP error %d", response.status));
     else if (response.body.empty())
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
@@ -262,7 +262,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
             item.release(); /* if true, queue took ownership */
         else {
             LogPrintf("WARNING: request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting\n");
-            item->req->WriteReply(HTTP_INTERNAL_SERVER_ERROR, "Work queue depth exceeded");
+            item->req->WriteReply(HTTP_SERVICE_UNAVAILABLE, "Work queue depth exceeded");
         }
     } else {
         hreq->WriteReply(HTTP_NOT_FOUND);
diff --git a/test/functional/interface_rpc.py b/test/functional/interface_rpc.py
@@ -8,6 +8,9 @@
 from test_framework.authproxy import JSONRPCException
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, assert_greater_than_or_equal
+from threading import Thread
+import subprocess
+
 
 def expect_http_status(expected_http_status, expected_rpc_code,
                        fcn, *args):
@@ -18,6 +21,16 @@ def expect_http_status(expected_http_status, expected_rpc_code,
         assert_equal(exc.error["code"], expected_rpc_code)
         assert_equal(exc.http_status, expected_http_status)
 
+
+def test_work_queue_getblock(node, got_exceeded_error):
+    while not got_exceeded_error:
+        try:
+            node.cli('getrpcinfo').send_cli()
+        except subprocess.CalledProcessError as e:
+            assert_equal(e.output, 'error: Server response: Work queue depth exceeded\n')
+            got_exceeded_error.append(True)
+
+
 class RPCInterfaceTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
@@ -67,10 +80,23 @@ def test_http_status_codes(self):
         expect_http_status(404, -32601, self.nodes[0].invalidmethod)
         expect_http_status(500, -8, self.nodes[0].getblockhash, 42)
 
+    def test_work_queue_exceeded(self):
+        self.log.info("Testing work queue exceeded...")
+        self.restart_node(0, ['-rpcworkqueue=1', '-rpcthreads=1'])
+        got_exceeded_error = []
+        threads = []
+        for _ in range(3):
+            t = Thread(target=test_work_queue_getblock, args=(self.nodes[0], got_exceeded_error))
+            t.start()
+            threads.append(t)
+        for t in threads:
+            t.join()
+
     def run_test(self):
         self.test_getrpcinfo()
         self.test_batch_request()
         self.test_http_status_codes()
+        self.test_work_queue_exceeded()
 
 
 if __name__ == '__main__':

Original file line number	Diff line number	Diff line change
`@@ -708,6 +708,8 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co`
`708`	`708`	`} else {`
`709`	`709`	`throw std::runtime_error("Authorization failed: Incorrect rpcuser or rpcpassword");`
`710`	`710`	`}`
	`711`	`+ } else if (response.status == HTTP_SERVICE_UNAVAILABLE) {`
	`712`	`+ throw std::runtime_error(strprintf("Server response: %s", response.body));`
`711`	`713`	`} else if (response.status >= 400 && response.status != HTTP_BAD_REQUEST && response.status != HTTP_NOT_FOUND && response.status != HTTP_INTERNAL_SERVER_ERROR)`
`712`	`714`	`throw std::runtime_error(strprintf("server returned HTTP error %d", response.status));`
`713`	`715`	`else if (response.body.empty())`
Original file line number	Diff line number	Diff line change
`@@ -262,7 +262,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)`
`262`	`262`	`item.release(); /* if true, queue took ownership */`
`263`	`263`	`else {`
`264`	`264`	`LogPrintf("WARNING: request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting\n");`
`265`		`- item->req->WriteReply(HTTP_INTERNAL_SERVER_ERROR, "Work queue depth exceeded");`
	`265`	`+ item->req->WriteReply(HTTP_SERVICE_UNAVAILABLE, "Work queue depth exceeded");`
`266`	`266`	`}`
`267`	`267`	`} else {`
`268`	`268`	`hreq->WriteReply(HTTP_NOT_FOUND);`