From 48a5e07ad833bd1a8fcb2ce6f85a41ad0cef9dc6 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 24 Aug 2024 13:31:05 +0100 Subject: [PATCH] Return 500 error when handler has wrong return type (#8845) --- CHANGES/8845.bugfix.rst | 1 + aiohttp/web_protocol.py | 36 ++++++++++++++++++------------------ tests/test_web_functional.py | 17 ++++------------- tests/test_web_request.py | 17 ----------------- 4 files changed, 23 insertions(+), 48 deletions(-) create mode 100644 CHANGES/8845.bugfix.rst diff --git a/CHANGES/8845.bugfix.rst b/CHANGES/8845.bugfix.rst new file mode 100644 index 00000000000..ff0016ac14b --- /dev/null +++ b/CHANGES/8845.bugfix.rst @@ -0,0 +1 @@ +Changed behaviour when returning an invalid response to send a 500 response -- by :user:`Dreamsorcerer`. diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 5e12d611ae2..e0c174e0649 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -39,7 +39,7 @@ from .log import access_logger, server_logger from .streams import EMPTY_PAYLOAD, StreamReader from .tcp_helpers import tcp_keepalive -from .web_exceptions import HTTPException +from .web_exceptions import HTTPException, HTTPInternalServerError from .web_log import AccessLogger from .web_request import BaseRequest from .web_response import Response, StreamResponse @@ -490,18 +490,18 @@ async def _handle_request( status=exc.status, reason=exc.reason, text=exc.text, headers=exc.headers ) resp._cookies = exc._cookies - reset = await self.finish_response(request, resp, start_time) + resp, reset = await self.finish_response(request, resp, start_time) except asyncio.CancelledError: raise except asyncio.TimeoutError as exc: self.log_debug("Request handler timed out.", exc_info=exc) resp = self.handle_error(request, 504) - reset = await self.finish_response(request, resp, start_time) + resp, reset = await self.finish_response(request, resp, start_time) except Exception as exc: resp = self.handle_error(request, 500, exc) - reset = await self.finish_response(request, resp, start_time) + resp, reset = await self.finish_response(request, resp, start_time) else: - reset = await self.finish_response(request, resp, start_time) + resp, reset = await self.finish_response(request, resp, start_time) finally: self._handler_waiter.set_result(None) @@ -603,10 +603,6 @@ async def start(self) -> None: except asyncio.CancelledError: self.log_debug("Ignored premature client disconnection ") break - except RuntimeError as exc: - if self._loop.get_debug(): - self.log_exception("Unhandled runtime exception", exc_info=exc) - self.force_close() except Exception as exc: self.log_exception("Unhandled exception", exc_info=exc) self.force_close() @@ -635,7 +631,7 @@ async def start(self) -> None: async def finish_response( self, request: BaseRequest, resp: StreamResponse, start_time: float - ) -> bool: + ) -> Tuple[StreamResponse, bool]: """Prepare the response and write_eof, then log access. This has to @@ -654,22 +650,26 @@ async def finish_response( prepare_meth = resp.prepare except AttributeError: if resp is None: - raise RuntimeError("Missing return " "statement on request handler") + self.log_exception("Missing return statement on request handler") else: - raise RuntimeError( - "Web-handler should return " - "a response instance, " + self.log_exception( + "Web-handler should return a response instance, " "got {!r}".format(resp) ) + exc = HTTPInternalServerError() + resp = Response( + status=exc.status, reason=exc.reason, text=exc.text, headers=exc.headers + ) + prepare_meth = resp.prepare try: await prepare_meth(request) await resp.write_eof() except ConnectionError: await self.log_access(request, resp, start_time) - return True - else: - await self.log_access(request, resp, start_time) - return False + return resp, True + + await self.log_access(request, resp, start_time) + return resp, False def handle_error( self, diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index 565efc7b4ca..649b9b0237d 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -103,12 +103,8 @@ async def handler(request): server = await aiohttp_server(app, logger=logger) client = await aiohttp_client(server) - with pytest.raises(aiohttp.ServerDisconnectedError): - await client.get("/") - - logger.exception.assert_called_with( - "Unhandled runtime exception", exc_info=mock.ANY - ) + async with client.get("/") as resp: + assert resp.status == 500 async def test_handler_returns_none(aiohttp_server: Any, aiohttp_client: Any) -> None: @@ -123,13 +119,8 @@ async def handler(request): server = await aiohttp_server(app, logger=logger) client = await aiohttp_client(server) - with pytest.raises(aiohttp.ServerDisconnectedError): - await client.get("/") - - # Actual error text is placed in exc_info - logger.exception.assert_called_with( - "Unhandled runtime exception", exc_info=mock.ANY - ) + async with client.get("/") as resp: + assert resp.status == 500 async def test_head_returns_empty_body(aiohttp_client: Any) -> None: diff --git a/tests/test_web_request.py b/tests/test_web_request.py index 58394886534..b78c4811fa6 100644 --- a/tests/test_web_request.py +++ b/tests/test_web_request.py @@ -12,7 +12,6 @@ from yarl import URL from aiohttp import HttpVersion, web -from aiohttp.client_exceptions import ServerDisconnectedError from aiohttp.http_parser import RawRequestMessage from aiohttp.streams import StreamReader from aiohttp.test_utils import make_mocked_request @@ -820,22 +819,6 @@ def test_weakref_creation() -> None: weakref.ref(req) -@pytest.mark.xfail( - raises=ServerDisconnectedError, - reason="see https://github.com/aio-libs/aiohttp/issues/4572", -) -async def test_handler_return_type(aiohttp_client: Any) -> None: - async def invalid_handler_1(request): - return 1 - - app = web.Application() - app.router.add_get("/1", invalid_handler_1) - client = await aiohttp_client(app) - - async with client.get("/1") as resp: - assert 500 == resp.status - - @pytest.mark.parametrize( ["header", "header_attr"], [