diff --git a/doc/release-notes-35182.md b/doc/release-notes-35182.md index 8d2b691880a..c32a922cb2b 100644 --- a/doc/release-notes-35182.md +++ b/doc/release-notes-35182.md @@ -14,3 +14,6 @@ Certain HTTP edge cases will observe different behavior to be more RFC-compliant - "Line Folding" is rejected (whitespace at start of a header line) - Tolerate `%` at the end of requested URLs - Multiple "Content-Length" headers with different values are rejected + +A new configuration option `-rpcmaxconnections` (default `16`) limits the +number of simultaneously connected HTTP clients to the server. diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 373251df3d8..8b0486c43ff 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -952,7 +952,7 @@ void HTTPServer::SocketHandlerListening(const Sock::EventsPerSock& events_per_so // Drain all pending connections from this socket up to the limit. // Stop early if the kernel queue is empty (AcceptConnection returns null) // or if accepting the last connection brought us to the limit. - while (GetConnectionsCount() < MAX_HTTP_CONNECTIONS) { + while (GetConnectionsCount() < static_cast(m_rpcmaxconnections)) { CService addr_accepted; auto sock_accepted{AcceptConnection(*sock, addr_accepted)}; if (!sock_accepted) break; @@ -970,7 +970,7 @@ HTTPServer::IOReadiness HTTPServer::GenerateWaitSockets() const // don't bother checking the listening sockets for new inbound connections. // Leave them in the kernel's queue until space in the application opens // up (or the client times out on its own). - if (GetConnectionsCount() < MAX_HTTP_CONNECTIONS) { + if (GetConnectionsCount() < static_cast(m_rpcmaxconnections)) { for (const auto& sock : m_listen) { io_readiness.events_per_sock.emplace(sock, Sock::Events{Sock::RecvEvent}); } @@ -1284,6 +1284,7 @@ bool InitHTTPServer() } g_http_server->SetServerTimeout(std::chrono::seconds(gArgs.GetIntArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT))); + g_http_server->SetMaxConnections(std::max(gArgs.GetArg("-rpcmaxconnections", DEFAULT_MAX_HTTP_CONNECTIONS), 1)); // Bind HTTP server to specified addresses std::vector> endpoints{GetBindAddresses()}; diff --git a/src/httpserver.h b/src/httpserver.h index 6090959274f..ef51c84ea9a 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -44,7 +44,7 @@ inline constexpr int DEFAULT_HTTP_SERVER_TIMEOUT=30; /** * Maximum number of connected HTTP clients */ -inline constexpr int MAX_HTTP_CONNECTIONS = 16; +inline constexpr int DEFAULT_MAX_HTTP_CONNECTIONS = 16; enum class HTTPRequestMethod { UNKNOWN, @@ -316,6 +316,11 @@ public: */ void SetServerTimeout(std::chrono::seconds seconds) { m_rpcservertimeout = seconds; } + /** + * Set the maximum amount of connected HTTPClients (-rpcmaxconnections) + */ + void SetMaxConnections(int max_conn) { m_rpcmaxconnections = max_conn; } + /** * Force-remove all remaining clients from m_connected without waiting for * graceful disconnection. Must only be called after JoinSocketsThreads(). @@ -421,6 +426,11 @@ private: */ bool ClientAllowed(const CNetAddr& netaddr) const; + /** + * Maximum amount of concurrent connections + */ + int m_rpcmaxconnections{DEFAULT_MAX_HTTP_CONNECTIONS}; + /** * Accept a connection. * @param[in] listen_sock Socket on which to accept the connection. diff --git a/src/init.cpp b/src/init.cpp index 13fa5e621a9..5f9e0b92daf 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -746,6 +746,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-rpcdoccheck", strprintf("Throw a non-fatal error at runtime if the documentation for an RPC is incorrect (default: %u)", DEFAULT_RPC_DOC_CHECK), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC); argsman.AddArg("-rpccookiefile=", "Location of the auth cookie. Relative paths will be prefixed by a net-specific datadir location. (default: data dir)", ArgsManager::ALLOW_ANY, OptionsCategory::RPC); argsman.AddArg("-rpccookieperms=", strprintf("Set permissions on the RPC auth cookie file so that it is readable by [owner|group|all] (default: owner [via umask 0077])"), ArgsManager::ALLOW_ANY, OptionsCategory::RPC); + argsman.AddArg("-rpcmaxconnections=", strprintf("The maximum number of connected HTTP clients (default: %d)", DEFAULT_MAX_HTTP_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::RPC); argsman.AddArg("-rpcpassword=", "Password for JSON-RPC connections", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC); argsman.AddArg("-rpcport=", strprintf("Listen for JSON-RPC connections on (default: %u, testnet3: %u, testnet4: %u, signet: %u, regtest: %u)", defaultBaseParams->RPCPort(), testnetBaseParams->RPCPort(), testnet4BaseParams->RPCPort(), signetBaseParams->RPCPort(), regtestBaseParams->RPCPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::RPC); argsman.AddArg("-rpcservertimeout=", strprintf("Timeout during HTTP requests (default: %d)", DEFAULT_HTTP_SERVER_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC); diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py index ba7bcc75daa..58ee6289ac4 100755 --- a/test/functional/interface_http.py +++ b/test/functional/interface_http.py @@ -8,6 +8,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.netutil import NETWORK_ERRORS from test_framework.util import assert_equal, str_to_b64str +import concurrent.futures import http.client import socket import threading @@ -615,44 +616,78 @@ class HTTPBasicsTest (BitcoinTestFramework): # Disable timeout so the initial batch of clients stays connected # until the end of the test. - self.restart_node(0, extra_args=["-rpcservertimeout=0"]) + for comment, extra_args, limit in [ + ("default (16)", ["-rpcservertimeout=0", "-rest"], 16), + ("-rpcmaxconnections=128", ["-rpcservertimeout=0", "-rest", "-rpcmaxconnections=128"], 128) + ]: + self.log.info(f"Using connection limit: {comment}") + self.restart_node(0, extra_args=extra_args) - # Close the persistent HTTP connection to this node by replacing it with - # a new AuthServiceProxy, reducing HTTPServer::GetConnectionsCount() to 0. - # The new AuthServiceProxy won't actually open an HTTP connection until - # it needs to send an RPC (for example, to stop the node at the end of the test). - self.node._rpc = self.node.create_new_rpc_connection(mode="AUTHPROXY") + # Close the persistent HTTP connection to this node by replacing it with + # a new AuthServiceProxy, reducing HTTPServer::GetConnectionsCount() to 0. + # The new AuthServiceProxy won't actually open an HTTP connection until + # it needs to send an RPC (for example, to stop the node at the end of the test). + self.node._rpc = self.node.create_new_rpc_connection(mode="AUTHPROXY") - MAX_HTTP_CONNECTIONS = 16 - connections = [] + MAX_HTTP_CONNECTIONS = limit + connections = [] - # Connections all succeed up to the limit - with self.node.assert_debug_log( - expected_msgs = [f"method=invalidrpc_{i}" for i in range(1, MAX_HTTP_CONNECTIONS + 1)] - ): - for i in range(1, MAX_HTTP_CONNECTIONS + 1): + # Connections all succeed up to the limit + with self.node.assert_debug_log( + expected_msgs = [f"method=invalidrpc_{i} " for i in range(1, MAX_HTTP_CONNECTIONS + 1)] + ): + for i in range(1, MAX_HTTP_CONNECTIONS + 1): + conn = BitcoinHTTPConnection(self.node) + # Each client makes a unique request so it's easy to find in the log + conn.post('/', f'{{"method": "invalidrpc_{i}"}}', connection_header='keep-alive').read() + connections.append(conn) + + # The next connection is over the limit, expect it to timeout + with self.node.assert_debug_log( + expected_msgs = [], + unexpected_msgs = ["method=never_accepted"] + ): conn = BitcoinHTTPConnection(self.node) - # Each client makes a unique request so it's easy to find in the log - conn.post('/', f'{{"method": "invalidrpc_{i}"}}', connection_header='keep-alive').read() - connections.append(conn) + conn.set_timeout(5) + try: + conn.post('/', '{"method": "never_accepted"}', connection_header='keep-alive').read() + assert False, "Connection succeeded unexpectedly" + except TimeoutError: + pass + + # All original clients are still connected + assert_equal(len(connections), MAX_HTTP_CONNECTIONS) + for client in connections: + assert not client.sock_closed() + + # Try connecting again, but this time we'll wait for acceptance. + # Because the send is blocking, we'll execute in a background thread. + + def wait_for_send(conn): + return conn.get('/rest/blockhashbyheight/0.json').read() - # The next connection is over the limit, expect it to timeout - with self.node.assert_debug_log( - expected_msgs = [], - unexpected_msgs = ["method=never_accepted"] - ): conn = BitcoinHTTPConnection(self.node) - conn.set_timeout(5) - try: - conn.post('/', '{"method": "never_accepted"}', connection_header='keep-alive').read() - assert False, "Connection succeeded unexpectedly" - except TimeoutError: - pass + conn.set_timeout(None) + executor = concurrent.futures.ThreadPoolExecutor(max_workers=1) + waiting_request = executor.submit( + wait_for_send, + conn + ) - # Original 16 clients are still connected - assert_equal(len(connections), MAX_HTTP_CONNECTIONS) - for client in connections: - assert not client.sock_closed() + # We are waiting + assert not waiting_request.done() + + # Close one of the original connections + popped_client = connections.pop() + popped_client.close_sock() + + # The waiting connection gets processed + delayed_response = waiting_request.result(timeout=5) + assert "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206" in delayed_response.decode() + + # Close all remaining connections for clean up + for client in connections: + client.close_sock() if __name__ == '__main__':