From 45c22f9af23132d00d77578efbc410999a6954e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=2E=20K=C3=A4rkk=C3=A4inen?= <98187+Tronic@users.noreply.github.com> Date: Tue, 16 Nov 2021 13:03:27 -0800 Subject: [PATCH] Make HTTP connections start in IDLE stage, avoiding delays and error messages (#2268) * Make all new connections start in IDLE stage, and switch to REQUEST stage only once any bytes are received from client. This makes new connections without any request obey keepalive timeout rather than request timeout like they currently do. * Revert typo * Remove request timeout endpoint test which is no longer working (still tested by mocking). Fix mock timeout test setup. Co-authored-by: L. Karkkainen --- sanic/http.py | 22 +++---- tests/test_request_timeout.py | 109 ---------------------------------- tests/test_timeout_logic.py | 1 + 3 files changed, 9 insertions(+), 123 deletions(-) delete mode 100644 tests/test_request_timeout.py diff --git a/sanic/http.py b/sanic/http.py index d30e4c82..6f59ef25 100644 --- a/sanic/http.py +++ b/sanic/http.py @@ -105,7 +105,6 @@ class Http(metaclass=TouchUpMeta): self.keep_alive = True self.stage: Stage = Stage.IDLE self.dispatch = self.protocol.app.dispatch - self.init_for_request() def init_for_request(self): """Init/reset all per-request variables.""" @@ -129,14 +128,20 @@ class Http(metaclass=TouchUpMeta): """ HTTP 1.1 connection handler """ - while True: # As long as connection stays keep-alive + # Handle requests while the connection stays reusable + while self.keep_alive and self.stage is Stage.IDLE: + self.init_for_request() + # Wait for incoming bytes (in IDLE stage) + if not self.recv_buffer: + await self._receive_more() + self.stage = Stage.REQUEST try: # Receive and handle a request - self.stage = Stage.REQUEST self.response_func = self.http1_response_header await self.http1_request_header() + self.stage = Stage.HANDLER self.request.conn_info = self.protocol.conn_info await self.protocol.request_handler(self.request) @@ -187,16 +192,6 @@ class Http(metaclass=TouchUpMeta): if self.response: self.response.stream = None - # Exit and disconnect if no more requests can be taken - if self.stage is not Stage.IDLE or not self.keep_alive: - break - - self.init_for_request() - - # Wait for the next request - if not self.recv_buffer: - await self._receive_more() - async def http1_request_header(self): # no cov """ Receive and parse request header into self.request. @@ -299,7 +294,6 @@ class Http(metaclass=TouchUpMeta): # Remove header and its trailing CRLF del buf[: pos + 4] - self.stage = Stage.HANDLER self.request, request.stream = request, self self.protocol.state["requests_count"] += 1 diff --git a/tests/test_request_timeout.py b/tests/test_request_timeout.py deleted file mode 100644 index 48e23f1d..00000000 --- a/tests/test_request_timeout.py +++ /dev/null @@ -1,109 +0,0 @@ -import asyncio - -import httpcore -import httpx -import pytest - -from sanic_testing.testing import SanicTestClient - -from sanic import Sanic -from sanic.response import text - - -class DelayableHTTPConnection(httpcore._async.connection.AsyncHTTPConnection): - async def arequest(self, *args, **kwargs): - await asyncio.sleep(2) - return await super().arequest(*args, **kwargs) - - async def _open_socket(self, *args, **kwargs): - retval = await super()._open_socket(*args, **kwargs) - if self._request_delay: - await asyncio.sleep(self._request_delay) - return retval - - -class DelayableSanicConnectionPool(httpcore.AsyncConnectionPool): - def __init__(self, request_delay=None, *args, **kwargs): - self._request_delay = request_delay - super().__init__(*args, **kwargs) - - async def _add_to_pool(self, connection, timeout): - connection.__class__ = DelayableHTTPConnection - connection._request_delay = self._request_delay - await super()._add_to_pool(connection, timeout) - - -class DelayableSanicSession(httpx.AsyncClient): - def __init__(self, request_delay=None, *args, **kwargs) -> None: - transport = DelayableSanicConnectionPool(request_delay=request_delay) - super().__init__(transport=transport, *args, **kwargs) - - -class DelayableSanicTestClient(SanicTestClient): - def __init__(self, app, request_delay=None): - super().__init__(app) - self._request_delay = request_delay - self._loop = None - - def get_new_session(self): - return DelayableSanicSession(request_delay=self._request_delay) - - -@pytest.fixture -def request_no_timeout_app(): - app = Sanic("test_request_no_timeout") - app.config.REQUEST_TIMEOUT = 0.6 - - @app.route("/1") - async def handler2(request): - return text("OK") - - return app - - -@pytest.fixture -def request_timeout_default_app(): - app = Sanic("test_request_timeout_default") - app.config.REQUEST_TIMEOUT = 0.6 - - @app.route("/1") - async def handler1(request): - return text("OK") - - @app.websocket("/ws1") - async def ws_handler1(request, ws): - await ws.send("OK") - - return app - - -def test_default_server_error_request_timeout(request_timeout_default_app): - client = DelayableSanicTestClient(request_timeout_default_app, 2) - _, response = client.get("/1") - assert response.status == 408 - assert "Request Timeout" in response.text - - -def test_default_server_error_request_dont_timeout(request_no_timeout_app): - client = DelayableSanicTestClient(request_no_timeout_app, 0.2) - _, response = client.get("/1") - assert response.status == 200 - assert response.text == "OK" - - -def test_default_server_error_websocket_request_timeout( - request_timeout_default_app, -): - - headers = { - "Upgrade": "websocket", - "Connection": "upgrade", - "Sec-WebSocket-Key": "dGhlIHNhbXBsZSBub25jZQ==", - "Sec-WebSocket-Version": "13", - } - - client = DelayableSanicTestClient(request_timeout_default_app, 2) - _, response = client.get("/ws1", headers=headers) - - assert response.status == 408 - assert "Request Timeout" in response.text diff --git a/tests/test_timeout_logic.py b/tests/test_timeout_logic.py index 05249f11..497deda9 100644 --- a/tests/test_timeout_logic.py +++ b/tests/test_timeout_logic.py @@ -26,6 +26,7 @@ def protocol(app, mock_transport): protocol = HttpProtocol(loop=loop, app=app) protocol.connection_made(mock_transport) protocol._setup_connection() + protocol._http.init_for_request() protocol._task = Mock(spec=asyncio.Task) protocol._task.cancel = Mock() return protocol