From e2b2daf0b3089793b1c8298e246ffc4fc5580561 Mon Sep 17 00:00:00 2001 From: Nicolas Limage Date: Fri, 27 Nov 2020 19:25:06 +0100 Subject: [PATCH] add missing shutdown_request (#1770) This patch fixes the error handling of cases where there are runtime errors in `socketserver`. For example, when creating a new thread (to handle a request) fails. `get_request` handles ssl connections by replacing the new client socket by a tuple containing `(server_socket, new_client_socket)` in order to later deal with handshakes in `process_request_thread` During the processing of a request, the socketserver `BaseServer` calls `handle_request`, calling the `_handle_request_noblock`, which is calling the following functions (https://github.com/python/cpython/blob/3.8/Lib/socketserver.py#L303): ``` request, client_addr = get_request() verify_request(request, client_address): process_request(request, client_address) handle_error(request, client_address) shutdown_request(request) ``` - `get_request` is overloaded in patroni and returns `request` as a tuple in case of ssl calls - `verify_request` defaults to `return True` and should be fixed if used but is fine in this case - `process_request` just calls `process_request_thread` (which is overloaded in patroni and handles tuple-style requests) - `handle_error` is overloaded in patroni and handles tuple-style requests) - but `shutdown_request` is not overloaded and thus missing support for tuple-style requests This patch adds support for tuple-style requests in patroni api --- patroni/api.py | 5 +++++ tests/test_api.py | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/patroni/api.py b/patroni/api.py index c596e0ec..d24bae96 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -664,6 +664,11 @@ class RestApiServer(ThreadingMixIn, HTTPServer, Thread): newsock = (sock, newsock) return newsock, addr + def shutdown_request(self, request): + if isinstance(request, tuple): + _, request = request # SSLSocket + return super(RestApiServer, self).shutdown_request(request) + def reload_config(self, config): if 'listen' not in config: # changing config in runtime raise ValueError('Can not find "restapi.listen" config') diff --git a/tests/test_api.py b/tests/test_api.py index 19c76859..a382fa6f 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -488,3 +488,16 @@ class TestRestApiServer(unittest.TestCase): mock_accept.return_value = (newsock, '2') self.srv.socket = Mock() self.assertEqual(self.srv.get_request(), ((self.srv.socket, newsock), '2')) + + @patch.object(MockRestApiServer, 'process_request', Mock(side_effect=RuntimeError)) + def test_process_request_error(self): + mock_address = ('127.0.0.1', 55555) + mock_socket = Mock() + mock_ssl_socket = (Mock(), Mock()) + for mock_request in (mock_socket, mock_ssl_socket): + with patch.object( + MockRestApiServer, + 'get_request', + Mock(return_value=(mock_request, mock_address)) + ): + self.srv._handle_request_noblock()