From ff6972f28ff1d06a530c3586cac9ea5c284d08e9 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 26 Nov 2021 15:58:25 -0600 Subject: [PATCH 01/62] Nicer error handling after eof, waning for second res created --- sanic/http.py | 5 +++++ sanic/request.py | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/sanic/http.py b/sanic/http.py index 6f59ef250f..ad62cf8357 100644 --- a/sanic/http.py +++ b/sanic/http.py @@ -15,6 +15,7 @@ HeaderExpectationFailed, InvalidUsage, PayloadTooLarge, + SanicException, ServerError, ServiceUnavailable, ) @@ -589,6 +590,10 @@ def respond(self, response: BaseHTTPResponse) -> BaseHTTPResponse: @property def send(self): + if self.response_func is None and self.stage == Stage.IDLE: + raise SanicException( + "Response stream was ended, no more response data is allowed to be sent." + ) return self.response_func @classmethod diff --git a/sanic/request.py b/sanic/request.py index 68c2725724..6f5fbe4afe 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -109,6 +109,7 @@ class Request: "stream", "transport", "version", + "response", ) def __init__( @@ -149,6 +150,7 @@ def __init__( self.parsed_not_grouped_args: DefaultDict[ Tuple[bool, bool, str, str], List[Tuple[str, str]] ] = defaultdict(list) + self.response: Optional[BaseHTTPResponse] = None self.request_middleware_started = False self._cookies: Optional[Dict[str, str]] = None self._match_info: Dict[str, Any] = {} @@ -172,6 +174,11 @@ async def respond( headers: Optional[Union[Header, Dict[str, str]]] = None, content_type: Optional[str] = None, ): + if self.response: + logger.warning( + "Another response instance was created before, please consider " + "re-use it rather than creating a new one." + ) # This logic of determining which response to use is subject to change if response is None: response = (self.stream and self.stream.response) or HTTPResponse( @@ -193,6 +200,7 @@ async def respond( error_logger.exception( "Exception occurred in one of response middleware handlers" ) + self.response = response return response async def receive_body(self): From 978412ae5844a51f2c767ab03fa8f92e101fc1e4 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 26 Nov 2021 22:58:52 -0600 Subject: [PATCH 02/62] Fix type hint --- sanic/response.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanic/response.py b/sanic/response.py index 1f1d7fbe97..1da4486a1d 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -101,7 +101,7 @@ def processed_headers(self) -> Iterator[Tuple[bytes, bytes]]: async def send( self, - data: Optional[Union[AnyStr]] = None, + data: Optional[AnyStr] = None, end_stream: Optional[bool] = None, ) -> None: """ From 7b4cd01167fdfc4ee6d2200d4e96e583906a2083 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 26 Nov 2021 23:18:49 -0600 Subject: [PATCH 03/62] Prevent second call to request.respond in handler. --- sanic/request.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sanic/request.py b/sanic/request.py index 6f5fbe4afe..652e3300af 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -32,7 +32,7 @@ from sanic.compat import CancelledErrors, Header from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE -from sanic.exceptions import InvalidUsage +from sanic.exceptions import InvalidUsage, SanicException from sanic.headers import ( AcceptContainer, Options, @@ -175,9 +175,9 @@ async def respond( content_type: Optional[str] = None, ): if self.response: - logger.warning( - "Another response instance was created before, please consider " - "re-use it rather than creating a new one." + raise SanicException( + "Another response instance was created, " + "creating the second response is not allowed for this request." ) # This logic of determining which response to use is subject to change if response is None: From d6e38af6a516a666f8b74a87315f3c5404a12b5e Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 26 Nov 2021 23:27:01 -0600 Subject: [PATCH 04/62] Move stream state detect logic from http to response --- sanic/http.py | 4 ---- sanic/response.py | 12 +++++++++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/sanic/http.py b/sanic/http.py index ad62cf8357..1e36be03d2 100644 --- a/sanic/http.py +++ b/sanic/http.py @@ -590,10 +590,6 @@ def respond(self, response: BaseHTTPResponse) -> BaseHTTPResponse: @property def send(self): - if self.response_func is None and self.stage == Stage.IDLE: - raise SanicException( - "Response stream was ended, no more response data is allowed to be sent." - ) return self.response_func @classmethod diff --git a/sanic/response.py b/sanic/response.py index 1da4486a1d..5859b7d0b3 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -19,8 +19,9 @@ from sanic.compat import Header, open_async from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE from sanic.cookies import CookieJar +from sanic.exceptions import SanicException from sanic.helpers import has_message_body, remove_entity_headers -from sanic.http import Http +from sanic.http import Http, Stage from sanic.models.protocol_types import HTMLProtocol, Range @@ -112,8 +113,13 @@ async def send( """ if data is None and end_stream is None: end_stream = True - if end_stream and not data and self.stream.send is None: - return + if self.stream.send is None: + if end_stream and not data: + return + elif self.stream.stage == Stage.IDLE: + raise SanicException( + "Response stream was ended, no more response data is allowed to be sent." + ) data = ( data.encode() # type: ignore if hasattr(data, "encode") From 584c5bc87c9a2f0355897f19eaab1ab6866f83cb Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Sun, 28 Nov 2021 00:21:50 -0600 Subject: [PATCH 05/62] Update exception handling for prevent second response --- sanic/app.py | 7 +++++++ sanic/exceptions.py | 11 +++++++++++ sanic/http.py | 1 - sanic/request.py | 39 +++++++++++++++++++++++++++++---------- sanic/response.py | 9 ++++++--- 5 files changed, 53 insertions(+), 14 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 6861bca013..c1b9c49865 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -63,6 +63,7 @@ InvalidUsage, SanicException, ServerError, + ShouldNotHandleException, URLBuildError, ) from sanic.handlers import ErrorHandler @@ -735,6 +736,12 @@ async def handle_exception( context={"request": request, "exception": exception}, ) + if isinstance(exception, ShouldNotHandleException): + error_logger.exception(exception) + return + + request.reset_response() + # -------------------------------------------- # # Request Middleware # -------------------------------------------- # diff --git a/sanic/exceptions.py b/sanic/exceptions.py index 6459f15a42..ab81062e25 100644 --- a/sanic/exceptions.py +++ b/sanic/exceptions.py @@ -266,3 +266,14 @@ def abort(status_code: int, message: Optional[Union[str, bytes]] = None): ) raise SanicException(message=message, status_code=status_code) + + +class ShouldNotHandleException(SanicException): + pass + + +class ResponseException(ShouldNotHandleException): + """ + Can be used when the response has already been sent and 500 error response + page won't be generated. + """ diff --git a/sanic/http.py b/sanic/http.py index 1e36be03d2..6f59ef250f 100644 --- a/sanic/http.py +++ b/sanic/http.py @@ -15,7 +15,6 @@ HeaderExpectationFailed, InvalidUsage, PayloadTooLarge, - SanicException, ServerError, ServiceUnavailable, ) diff --git a/sanic/request.py b/sanic/request.py index 652e3300af..abb7560488 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -18,7 +18,6 @@ if TYPE_CHECKING: from sanic.server import ConnInfo from sanic.app import Sanic - from sanic.http import Http import email.utils import uuid @@ -32,7 +31,7 @@ from sanic.compat import CancelledErrors, Header from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE -from sanic.exceptions import InvalidUsage, SanicException +from sanic.exceptions import InvalidUsage, ResponseException, SanicException from sanic.headers import ( AcceptContainer, Options, @@ -42,6 +41,7 @@ parse_host, parse_xforwarded, ) +from sanic.http import Http, Stage from sanic.log import error_logger, logger from sanic.models.protocol_types import TransportProtocol from sanic.response import BaseHTTPResponse, HTTPResponse @@ -109,7 +109,6 @@ class Request: "stream", "transport", "version", - "response", ) def __init__( @@ -150,7 +149,6 @@ def __init__( self.parsed_not_grouped_args: DefaultDict[ Tuple[bool, bool, str, str], List[Tuple[str, str]] ] = defaultdict(list) - self.response: Optional[BaseHTTPResponse] = None self.request_middleware_started = False self._cookies: Optional[Dict[str, str]] = None self._match_info: Dict[str, Any] = {} @@ -166,6 +164,15 @@ def __repr__(self): def generate_id(*_): return uuid.uuid4() + def reset_response(self): + if isinstance(self.stream, Http): + if self.stream.stage in (Stage.RESPONSE, Stage.IDLE): + raise ResponseException( + "Response can't be reset because it was already sent." + ) + elif self.stream.response: + self.stream.response = None + async def respond( self, response: Optional[BaseHTTPResponse] = None, @@ -174,11 +181,24 @@ async def respond( headers: Optional[Union[Header, Dict[str, str]]] = None, content_type: Optional[str] = None, ): - if self.response: - raise SanicException( - "Another response instance was created, " - "creating the second response is not allowed for this request." - ) + + if isinstance(self.stream, Http): + if self.stream.response: + exception = ( + ResponseException + if self.stream.stage in (Stage.RESPONSE, Stage.IDLE) + else SanicException + ) + raise exception( + "Another response was created for this request. " + "Creating the second response is not allowed. " + "The response of this request can be reset if not yet " + "sent." + ) + if self.stream.stage in (Stage.RESPONSE, Stage.IDLE): + raise ResponseException( + "Cannot send response to this request twice." + ) # This logic of determining which response to use is subject to change if response is None: response = (self.stream and self.stream.response) or HTTPResponse( @@ -200,7 +220,6 @@ async def respond( error_logger.exception( "Exception occurred in one of response middleware handlers" ) - self.response = response return response async def receive_body(self): diff --git a/sanic/response.py b/sanic/response.py index 5859b7d0b3..c5af758902 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -19,7 +19,7 @@ from sanic.compat import Header, open_async from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE from sanic.cookies import CookieJar -from sanic.exceptions import SanicException +from sanic.exceptions import ResponseException, SanicException from sanic.helpers import has_message_body, remove_entity_headers from sanic.http import Http, Stage from sanic.models.protocol_types import HTMLProtocol, Range @@ -117,9 +117,12 @@ async def send( if end_stream and not data: return elif self.stream.stage == Stage.IDLE: - raise SanicException( - "Response stream was ended, no more response data is allowed to be sent." + raise ResponseException( + "Response stream was ended, no more response data is" + " allow ed to be sent." ) + else: + raise SanicException("Send response function isn't available") data = ( data.encode() # type: ignore if hasattr(data, "encode") From 7af89efb644996a8ff1bdb6eea1e34a9a348f62c Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Sun, 28 Nov 2021 00:35:15 -0600 Subject: [PATCH 06/62] Add test cases --- tests/test_middleware.py | 25 +++++++++++++++++++++++++ tests/test_requests.py | 31 ++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index c19386e7e4..afc344d576 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -297,3 +297,28 @@ async def handler(request): _, response = app.test_client.get("/") assert response.json["foo"] == "bar" + + +def test_middleware_added_response(app): + response_middleware_run_count = 0 + request_middleware_run_count = 0 + + @app.on_response + def response(_, response): + nonlocal response_middleware_run_count + response_middleware_run_count += 1 + + @app.on_request + def request(_): + nonlocal request_middleware_run_count + request_middleware_run_count += 1 + + @app.get("/") + async def handler(request): + resp1 = await request.respond() + await resp1.eof() + return text("") + + _, response = app.test_client.get("/") + assert response_middleware_run_count == 1 + assert request_middleware_run_count == 1 diff --git a/tests/test_requests.py b/tests/test_requests.py index e5db9d20db..dde2e23d02 100644 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -16,7 +16,7 @@ from sanic import Blueprint, Sanic from sanic.exceptions import ServerError -from sanic.request import DEFAULT_HTTP_CONTENT_TYPE, RequestParameters +from sanic.request import DEFAULT_HTTP_CONTENT_TYPE, Request, RequestParameters from sanic.response import html, json, text @@ -2174,3 +2174,32 @@ def handler(request, **kwargs): _, response = app.test_client.post("/long/sub/route") assert response.status == 200 assert response.json == {} + + +def test_second_response(app): + @app.exception(ServerError) + def handler_exception(request, exception): + return text("Internal Server Error.", 500) + + @app.get("/") + async def two_responses(request: Request): + resp1 = await request.respond() + resp2 = await request.respond() + + request, response = app.test_client.get("/") + assert response.status == 500 + + +@pytest.mark.asyncio +async def test_second_response_asgi(app): + @app.exception(ServerError) + def handler_exception(request, exception): + return text("Internal Server Error.", 500) + + @app.get("/") + async def two_responses(request: Request): + resp1 = await request.respond() + resp2 = await request.respond() + + request, response = await app.asgi_client.get("/") + assert response.status == 500 From f3c9dd65f09d7dc6642b2bbbfe0a1f8b9d30a820 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Tue, 30 Nov 2021 18:09:37 -0600 Subject: [PATCH 07/62] Clean up new test cases --- tests/test_requests.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/test_requests.py b/tests/test_requests.py index dde2e23d02..78b58c0f52 100644 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -2177,10 +2177,6 @@ def handler(request, **kwargs): def test_second_response(app): - @app.exception(ServerError) - def handler_exception(request, exception): - return text("Internal Server Error.", 500) - @app.get("/") async def two_responses(request: Request): resp1 = await request.respond() @@ -2192,10 +2188,6 @@ async def two_responses(request: Request): @pytest.mark.asyncio async def test_second_response_asgi(app): - @app.exception(ServerError) - def handler_exception(request, exception): - return text("Internal Server Error.", 500) - @app.get("/") async def two_responses(request: Request): resp1 = await request.respond() From c79cb8eccb1693064d439325343593a1e617ab43 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Tue, 30 Nov 2021 19:58:11 -0600 Subject: [PATCH 08/62] handle exception in request.reset_response --- sanic/app.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index c1b9c49865..51b9c7794b 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -61,6 +61,7 @@ from sanic.config import SANIC_PREFIX, Config from sanic.exceptions import ( InvalidUsage, + ResponseException, SanicException, ServerError, ShouldNotHandleException, @@ -739,8 +740,16 @@ async def handle_exception( if isinstance(exception, ShouldNotHandleException): error_logger.exception(exception) return - - request.reset_response() + try: + request.reset_response() + except ResponseException as _: + error_logger.exception(exception) + logger.error( + "Server error page was not sent to the client for the " + "exception above because a previous response has been " + "sent at least partially." + ) + return # -------------------------------------------- # # Request Middleware From 7cc05dd00bcb9ddeab62c91e4fe91ec0e82acfed Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 13:58:48 -0600 Subject: [PATCH 09/62] More clear exception raising logic in Request.respond --- sanic/request.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/sanic/request.py b/sanic/request.py index abb7560488..d812bd65bb 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -189,16 +189,7 @@ async def respond( if self.stream.stage in (Stage.RESPONSE, Stage.IDLE) else SanicException ) - raise exception( - "Another response was created for this request. " - "Creating the second response is not allowed. " - "The response of this request can be reset if not yet " - "sent." - ) - if self.stream.stage in (Stage.RESPONSE, Stage.IDLE): - raise ResponseException( - "Cannot send response to this request twice." - ) + raise exception("Cannot send response to this request twice.") # This logic of determining which response to use is subject to change if response is None: response = (self.stream and self.stream.response) or HTTPResponse( From 23956ed3f9f1cdfa3ee5fbc2a5b098b40c53227f Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 15:53:05 -0600 Subject: [PATCH 10/62] Cleanup and make app doesn't send err page when another res was sent --- sanic/app.py | 28 ++++++++++++++++------------ sanic/exceptions.py | 11 ----------- sanic/request.py | 19 +------------------ sanic/response.py | 5 +++-- 4 files changed, 20 insertions(+), 43 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 51b9c7794b..27ebfdbb97 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -61,13 +61,12 @@ from sanic.config import SANIC_PREFIX, Config from sanic.exceptions import ( InvalidUsage, - ResponseException, SanicException, ServerError, - ShouldNotHandleException, URLBuildError, ) from sanic.handlers import ErrorHandler +from sanic.http import Http, Stage from sanic.log import LOGGING_CONFIG_DEFAULTS, Colors, error_logger, logger from sanic.mixins.listeners import ListenerEvent from sanic.models.futures import ( @@ -737,17 +736,15 @@ async def handle_exception( context={"request": request, "exception": exception}, ) - if isinstance(exception, ShouldNotHandleException): - error_logger.exception(exception) - return - try: - request.reset_response() - except ResponseException as _: + if isinstance(request.stream, Http) and request.stream.stage in ( + Stage.RESPONSE, + Stage.IDLE, + ): error_logger.exception(exception) logger.error( - "Server error page was not sent to the client for the " - "exception above because a previous response has been " - "sent at least partially." + "The error page was not sent to the client for the " + "exception raised above because a previous response has " + "already been sent at least partially." ) return @@ -1290,8 +1287,15 @@ async def _run_request_middleware( return None async def _run_response_middleware( - self, request, response, request_name=None + self, + request: Request, + response: BaseHTTPResponse, + request_name: Optional[str] = None, ): # no cov + if response.middlewares_ran: + return response + else: + response.middlewares_ran = True named_middleware = self.named_response_middleware.get( request_name, deque() ) diff --git a/sanic/exceptions.py b/sanic/exceptions.py index ab81062e25..6459f15a42 100644 --- a/sanic/exceptions.py +++ b/sanic/exceptions.py @@ -266,14 +266,3 @@ def abort(status_code: int, message: Optional[Union[str, bytes]] = None): ) raise SanicException(message=message, status_code=status_code) - - -class ShouldNotHandleException(SanicException): - pass - - -class ResponseException(ShouldNotHandleException): - """ - Can be used when the response has already been sent and 500 error response - page won't be generated. - """ diff --git a/sanic/request.py b/sanic/request.py index d812bd65bb..bade8fc36c 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -31,7 +31,7 @@ from sanic.compat import CancelledErrors, Header from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE -from sanic.exceptions import InvalidUsage, ResponseException, SanicException +from sanic.exceptions import InvalidUsage from sanic.headers import ( AcceptContainer, Options, @@ -164,15 +164,6 @@ def __repr__(self): def generate_id(*_): return uuid.uuid4() - def reset_response(self): - if isinstance(self.stream, Http): - if self.stream.stage in (Stage.RESPONSE, Stage.IDLE): - raise ResponseException( - "Response can't be reset because it was already sent." - ) - elif self.stream.response: - self.stream.response = None - async def respond( self, response: Optional[BaseHTTPResponse] = None, @@ -182,14 +173,6 @@ async def respond( content_type: Optional[str] = None, ): - if isinstance(self.stream, Http): - if self.stream.response: - exception = ( - ResponseException - if self.stream.stage in (Stage.RESPONSE, Stage.IDLE) - else SanicException - ) - raise exception("Cannot send response to this request twice.") # This logic of determining which response to use is subject to change if response is None: response = (self.stream and self.stream.response) or HTTPResponse( diff --git a/sanic/response.py b/sanic/response.py index c5af758902..30f2aaf582 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -19,7 +19,7 @@ from sanic.compat import Header, open_async from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE from sanic.cookies import CookieJar -from sanic.exceptions import ResponseException, SanicException +from sanic.exceptions import SanicException from sanic.helpers import has_message_body, remove_entity_headers from sanic.http import Http, Stage from sanic.models.protocol_types import HTMLProtocol, Range @@ -50,6 +50,7 @@ def __init__(self): self.status: int = None self.headers = Header({}) self._cookies: Optional[CookieJar] = None + self.middlewares_ran: bool = True def _encode_body(self, data: Optional[AnyStr]): if data is None: @@ -117,7 +118,7 @@ async def send( if end_stream and not data: return elif self.stream.stage == Stage.IDLE: - raise ResponseException( + raise SanicException( "Response stream was ended, no more response data is" " allow ed to be sent." ) From bc873f1aaa83428aa532437837664b07312c2364 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 15:54:42 -0600 Subject: [PATCH 11/62] Fix test case --- tests/test_requests.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_requests.py b/tests/test_requests.py index 78b58c0f52..a8a969c919 100644 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -2180,6 +2180,7 @@ def test_second_response(app): @app.get("/") async def two_responses(request: Request): resp1 = await request.respond() + resp1.send("some msg") resp2 = await request.respond() request, response = app.test_client.get("/") @@ -2191,6 +2192,7 @@ async def test_second_response_asgi(app): @app.get("/") async def two_responses(request: Request): resp1 = await request.respond() + resp1.send("some msg") resp2 = await request.respond() request, response = await app.asgi_client.get("/") From 25e0eb099c6298f6ba439a05403dd6068dc45d76 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 15:57:29 -0600 Subject: [PATCH 12/62] Fix --- sanic/app.py | 4 ++-- sanic/response.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 27ebfdbb97..5a9ee5d42a 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -1292,10 +1292,10 @@ async def _run_response_middleware( response: BaseHTTPResponse, request_name: Optional[str] = None, ): # no cov - if response.middlewares_ran: + if response.middlewares_executed: return response else: - response.middlewares_ran = True + response.middlewares_executed = True named_middleware = self.named_response_middleware.get( request_name, deque() ) diff --git a/sanic/response.py b/sanic/response.py index 30f2aaf582..d7465a53a9 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -50,7 +50,7 @@ def __init__(self): self.status: int = None self.headers = Header({}) self._cookies: Optional[CookieJar] = None - self.middlewares_ran: bool = True + self.middlewares_executed: bool = False def _encode_body(self, data: Optional[AnyStr]): if data is None: From df9c404f5feb3100669d6519676ef53b1270c28f Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 17:22:43 -0600 Subject: [PATCH 13/62] raise an exception when request.respond is called and another res was sent --- sanic/request.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sanic/request.py b/sanic/request.py index bade8fc36c..658eab3ee3 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -31,7 +31,7 @@ from sanic.compat import CancelledErrors, Header from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE -from sanic.exceptions import InvalidUsage +from sanic.exceptions import InvalidUsage, SanicException from sanic.headers import ( AcceptContainer, Options, @@ -172,7 +172,10 @@ async def respond( headers: Optional[Union[Header, Dict[str, str]]] = None, content_type: Optional[str] = None, ): - + if isinstance(self.stream, Http) and self.stream.stage is Stage.IDLE: + raise SanicException( + "Another response was sent previously." + ) # This logic of determining which response to use is subject to change if response is None: response = (self.stream and self.stream.response) or HTTPResponse( From 08c3eb441a113de3f279acb65d3df4268ba77adf Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 17:52:13 -0600 Subject: [PATCH 14/62] Remove broken tests --- tests/test_requests.py | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/tests/test_requests.py b/tests/test_requests.py index a8a969c919..68afc6c0b6 100644 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -15,7 +15,7 @@ ) from sanic import Blueprint, Sanic -from sanic.exceptions import ServerError +from sanic.exceptions import SanicException, ServerError from sanic.request import DEFAULT_HTTP_CONTENT_TYPE, Request, RequestParameters from sanic.response import html, json, text @@ -2175,25 +2175,3 @@ def handler(request, **kwargs): assert response.status == 200 assert response.json == {} - -def test_second_response(app): - @app.get("/") - async def two_responses(request: Request): - resp1 = await request.respond() - resp1.send("some msg") - resp2 = await request.respond() - - request, response = app.test_client.get("/") - assert response.status == 500 - - -@pytest.mark.asyncio -async def test_second_response_asgi(app): - @app.get("/") - async def two_responses(request: Request): - resp1 = await request.respond() - resp1.send("some msg") - resp2 = await request.respond() - - request, response = await app.asgi_client.get("/") - assert response.status == 500 From 94d8db99bac2ecfd97ff694c7a1a9e5873226bbb Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 17:53:33 -0600 Subject: [PATCH 15/62] Undo change --- sanic/app.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 5a9ee5d42a..953101ac73 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -1287,10 +1287,7 @@ async def _run_request_middleware( return None async def _run_response_middleware( - self, - request: Request, - response: BaseHTTPResponse, - request_name: Optional[str] = None, + self, request, response, request_name=None ): # no cov if response.middlewares_executed: return response From 8fe123b9b282551c3847954818c5fdb60055d6f3 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 20:42:06 -0600 Subject: [PATCH 16/62] Create new exception type: ResponseException; fixed msgs --- sanic/app.py | 4 ++-- sanic/exceptions.py | 4 ++++ sanic/request.py | 6 ++---- sanic/response.py | 6 +++--- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 953101ac73..d40390998d 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -742,8 +742,8 @@ async def handle_exception( ): error_logger.exception(exception) logger.error( - "The error page was not sent to the client for the " - "exception raised above because a previous response has " + f"The error response won't be sent to the client for the " + "{exception} because a previous response has " "already been sent at least partially." ) return diff --git a/sanic/exceptions.py b/sanic/exceptions.py index 6459f15a42..f811326d7b 100644 --- a/sanic/exceptions.py +++ b/sanic/exceptions.py @@ -266,3 +266,7 @@ def abort(status_code: int, message: Optional[Union[str, bytes]] = None): ) raise SanicException(message=message, status_code=status_code) + + +class ResponseException(SanicException): + pass diff --git a/sanic/request.py b/sanic/request.py index 658eab3ee3..b0f94b38ed 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -31,7 +31,7 @@ from sanic.compat import CancelledErrors, Header from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE -from sanic.exceptions import InvalidUsage, SanicException +from sanic.exceptions import InvalidUsage, ResponseException from sanic.headers import ( AcceptContainer, Options, @@ -173,9 +173,7 @@ async def respond( content_type: Optional[str] = None, ): if isinstance(self.stream, Http) and self.stream.stage is Stage.IDLE: - raise SanicException( - "Another response was sent previously." - ) + raise ResponseException("Another response was sent previously.") # This logic of determining which response to use is subject to change if response is None: response = (self.stream and self.stream.response) or HTTPResponse( diff --git a/sanic/response.py b/sanic/response.py index d7465a53a9..baa6960fc0 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -19,7 +19,7 @@ from sanic.compat import Header, open_async from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE from sanic.cookies import CookieJar -from sanic.exceptions import SanicException +from sanic.exceptions import ResponseException, SanicException from sanic.helpers import has_message_body, remove_entity_headers from sanic.http import Http, Stage from sanic.models.protocol_types import HTMLProtocol, Range @@ -118,9 +118,9 @@ async def send( if end_stream and not data: return elif self.stream.stage == Stage.IDLE: - raise SanicException( + raise ResponseException( "Response stream was ended, no more response data is" - " allow ed to be sent." + " allowed to be sent." ) else: raise SanicException("Send response function isn't available") From bb83527e8be8f6188404b4545cdcb15ca4e3eee9 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 20:42:57 -0600 Subject: [PATCH 17/62] Fix msg --- sanic/response.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sanic/response.py b/sanic/response.py index baa6960fc0..a38b3424fc 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -119,8 +119,8 @@ async def send( return elif self.stream.stage == Stage.IDLE: raise ResponseException( - "Response stream was ended, no more response data is" - " allowed to be sent." + "Response stream was ended, no more response data is " + "allowed to be sent." ) else: raise SanicException("Send response function isn't available") From 8ee92700df070fa7ad127ac982ec79293c33a548 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 21:41:02 -0600 Subject: [PATCH 18/62] Only run res middleware when res is new --- sanic/app.py | 4 ---- sanic/request.py | 23 +++++++++++++---------- sanic/response.py | 1 - 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index d40390998d..85985a87ba 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -1289,10 +1289,6 @@ async def _run_request_middleware( async def _run_response_middleware( self, request, response, request_name=None ): # no cov - if response.middlewares_executed: - return response - else: - response.middlewares_executed = True named_middleware = self.named_response_middleware.get( request_name, deque() ) diff --git a/sanic/request.py b/sanic/request.py index b0f94b38ed..a41964b8d6 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -175,26 +175,29 @@ async def respond( if isinstance(self.stream, Http) and self.stream.stage is Stage.IDLE: raise ResponseException("Another response was sent previously.") # This logic of determining which response to use is subject to change + re_use_res = False if response is None: response = (self.stream and self.stream.response) or HTTPResponse( status=status, headers=headers, content_type=content_type, ) + re_use_res = response is (self.stream and self.stream.response) # Connect the response if isinstance(response, BaseHTTPResponse) and self.stream: response = self.stream.respond(response) # Run response middleware - try: - response = await self.app._run_response_middleware( - self, response, request_name=self.name - ) - except CancelledErrors: - raise - except Exception: - error_logger.exception( - "Exception occurred in one of response middleware handlers" - ) + if not re_use_res: + try: + response = await self.app._run_response_middleware( + self, response, request_name=self.name + ) + except CancelledErrors: + raise + except Exception: + error_logger.exception( + "Exception occurred in one of response middleware handlers" + ) return response async def receive_body(self): diff --git a/sanic/response.py b/sanic/response.py index a38b3424fc..b092cee214 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -50,7 +50,6 @@ def __init__(self): self.status: int = None self.headers = Header({}) self._cookies: Optional[CookieJar] = None - self.middlewares_executed: bool = False def _encode_body(self, data: Optional[AnyStr]): if data is None: From b65ee67b9bb1d165e14d475a0f0fb28595f87018 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 22:00:44 -0600 Subject: [PATCH 19/62] Update test case --- tests/test_middleware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index afc344d576..9bb15202ab 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -316,8 +316,8 @@ def request(_): @app.get("/") async def handler(request): resp1 = await request.respond() + resp2 = await request.respond() await resp1.eof() - return text("") _, response = app.test_client.get("/") assert response_middleware_run_count == 1 From 83b4aba1c7c11234aa5efaa3b7809f8339160dda Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 22:16:19 -0600 Subject: [PATCH 20/62] Fix msg --- sanic/app.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 85985a87ba..c4c1a8235b 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -742,8 +742,8 @@ async def handle_exception( ): error_logger.exception(exception) logger.error( - f"The error response won't be sent to the client for the " - "{exception} because a previous response has " + "The error response won't be sent to the client for the " + f"{exception} because a previous response has " "already been sent at least partially." ) return From 777e9f6b0953c311f4c2c85db92d3428dc443b8a Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 22:17:46 -0600 Subject: [PATCH 21/62] Fix msg --- sanic/app.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index c4c1a8235b..13b8f328f3 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -742,8 +742,8 @@ async def handle_exception( ): error_logger.exception(exception) logger.error( - "The error response won't be sent to the client for the " - f"{exception} because a previous response has " + "The error response won't be sent to the client for the exception: " + f"\"{exception}\" because a previous response has " "already been sent at least partially." ) return From 3dab5fc2946179c7a493b697c172b85246dc51ae Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 23:16:12 -0600 Subject: [PATCH 22/62] Fix re_use_res --- sanic/request.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sanic/request.py b/sanic/request.py index a41964b8d6..0efc3b74eb 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -175,14 +175,17 @@ async def respond( if isinstance(self.stream, Http) and self.stream.stage is Stage.IDLE: raise ResponseException("Another response was sent previously.") # This logic of determining which response to use is subject to change - re_use_res = False if response is None: response = (self.stream and self.stream.response) or HTTPResponse( status=status, headers=headers, content_type=content_type, ) - re_use_res = response is (self.stream and self.stream.response) + re_use_res = ( + response is (self.stream and self.stream.response) + if isinstance(self.stream, Http) + else False + ) # Connect the response if isinstance(response, BaseHTTPResponse) and self.stream: response = self.stream.respond(response) From be9b4cd76f7b0c1f553c9b361bffe4e83efa4cb6 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 1 Dec 2021 23:18:35 -0600 Subject: [PATCH 23/62] Fix test case --- tests/test_middleware.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 9bb15202ab..408946852f 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -316,8 +316,7 @@ def request(_): @app.get("/") async def handler(request): resp1 = await request.respond() - resp2 = await request.respond() - await resp1.eof() + return resp1 _, response = app.test_client.get("/") assert response_middleware_run_count == 1 From 153be3ccc0d4821f27293462199dffe4b51c6e11 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 2 Dec 2021 00:14:23 -0600 Subject: [PATCH 24/62] Refactor some logiscs; other minor changes --- sanic/app.py | 2 +- sanic/request.py | 19 ++++++++++++------- tests/test_middleware.py | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 13b8f328f3..8ff2e41d30 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -743,7 +743,7 @@ async def handle_exception( error_logger.exception(exception) logger.error( "The error response won't be sent to the client for the exception: " - f"\"{exception}\" because a previous response has " + f'"{exception}" because a previous response has ' "already been sent at least partially." ) return diff --git a/sanic/request.py b/sanic/request.py index 0efc3b74eb..aebbfb2853 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -172,8 +172,6 @@ async def respond( headers: Optional[Union[Header, Dict[str, str]]] = None, content_type: Optional[str] = None, ): - if isinstance(self.stream, Http) and self.stream.stage is Stage.IDLE: - raise ResponseException("Another response was sent previously.") # This logic of determining which response to use is subject to change if response is None: response = (self.stream and self.stream.response) or HTTPResponse( @@ -181,11 +179,18 @@ async def respond( headers=headers, content_type=content_type, ) - re_use_res = ( - response is (self.stream and self.stream.response) - if isinstance(self.stream, Http) - else False - ) + + # Check the status of current stream and response. + re_use_res = False + try: + if self.stream.stage is Stage.IDLE: + raise ResponseException( + "Another response was sent previously." + ) + re_use_res = response is (self.stream and self.stream.response) + except AttributeError: + pass + # Connect the response if isinstance(response, BaseHTTPResponse) and self.stream: response = self.stream.respond(response) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 408946852f..2163e47c28 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -299,7 +299,7 @@ async def handler(request): assert response.json["foo"] == "bar" -def test_middleware_added_response(app): +def test_middleware_return_response(app): response_middleware_run_count = 0 request_middleware_run_count = 0 From 893d4329151060c246d5c16da5fad90f9c726668 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 2 Dec 2021 00:28:31 -0600 Subject: [PATCH 25/62] Fix type checking --- sanic/request.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sanic/request.py b/sanic/request.py index aebbfb2853..390aa4ee99 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -183,11 +183,12 @@ async def respond( # Check the status of current stream and response. re_use_res = False try: - if self.stream.stage is Stage.IDLE: - raise ResponseException( - "Another response was sent previously." - ) - re_use_res = response is (self.stream and self.stream.response) + if self.stream is not None: + if self.stream.stage is Stage.IDLE: + raise ResponseException( + "Another response was sent previously." + ) + re_use_res = response is (self.stream and self.stream.response) except AttributeError: pass From 8ca425fc7bffb49fd704e767d795208218e659f2 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 2 Dec 2021 02:14:58 -0600 Subject: [PATCH 26/62] Fix lint --- sanic/app.py | 4 ++-- sanic/request.py | 2 +- tests/test_requests.py | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 8ff2e41d30..8154f5c59f 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -742,8 +742,8 @@ async def handle_exception( ): error_logger.exception(exception) logger.error( - "The error response won't be sent to the client for the exception: " - f'"{exception}" because a previous response has ' + "The error response won't be sent to the client for the " + f'exception: "{exception}" because a previous response has ' "already been sent at least partially." ) return diff --git a/sanic/request.py b/sanic/request.py index 390aa4ee99..1796ea9b8d 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -179,7 +179,7 @@ async def respond( headers=headers, content_type=content_type, ) - + # Check the status of current stream and response. re_use_res = False try: diff --git a/tests/test_requests.py b/tests/test_requests.py index 68afc6c0b6..c8f6e3f016 100644 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -2174,4 +2174,3 @@ def handler(request, **kwargs): _, response = app.test_client.post("/long/sub/route") assert response.status == 200 assert response.json == {} - From 2c40907302d2e5cdba263ce76e8cc43dbc2892fa Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 2 Dec 2021 14:09:38 -0600 Subject: [PATCH 27/62] Candidate 2 implementation --- sanic/app.py | 9 ++++---- sanic/request.py | 55 ++++++++++++++++++++++++++++-------------------- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 8154f5c59f..ea4271a4d1 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -736,9 +736,9 @@ async def handle_exception( context={"request": request, "exception": exception}, ) - if isinstance(request.stream, Http) and request.stream.stage in ( - Stage.RESPONSE, - Stage.IDLE, + if ( + isinstance(request.stream, Http) + and request.stream.stage is not Stage.HANDLER ): error_logger.exception(exception) logger.error( @@ -777,6 +777,7 @@ async def handle_exception( ) if response is not None: try: + request.reset_response() response = await request.respond(response) except BaseException: # Skip response middleware @@ -886,7 +887,7 @@ async def handle_request(self, request: Request): # no cov if isawaitable(response): response = await response - if response is not None: + if response is not None and not request.responded: response = await request.respond(response) elif not hasattr(handler, "is_websocket"): response = request.stream.response # type: ignore diff --git a/sanic/request.py b/sanic/request.py index 1796ea9b8d..f5aac2c0aa 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -104,6 +104,7 @@ class Request: "parsed_json", "parsed_forwarded", "raw_url", + "responded", "request_middleware_started", "route", "stream", @@ -155,6 +156,7 @@ def __init__( self.stream: Optional[Http] = None self.route: Optional[Route] = None self._protocol = None + self.responded: bool = False def __repr__(self): class_name = self.__class__.__name__ @@ -164,6 +166,20 @@ def __repr__(self): def generate_id(*_): return uuid.uuid4() + def reset_response(self): + try: + if ( + self.stream is not None + and self.stream.stage is not Stage.HANDLER + ): + raise ResponseException( + "Cannot reset response because previous response was sent." + ) + self.stream.response = None + self.responded = False + except AttributeError: + pass + async def respond( self, response: Optional[BaseHTTPResponse] = None, @@ -172,6 +188,11 @@ async def respond( headers: Optional[Union[Header, Dict[str, str]]] = None, content_type: Optional[str] = None, ): + try: + if self.stream is not None and self.stream.response: + raise ResponseException("Second respond call is not allowed.") + except AttributeError: + pass # This logic of determining which response to use is subject to change if response is None: response = (self.stream and self.stream.response) or HTTPResponse( @@ -180,33 +201,21 @@ async def respond( content_type=content_type, ) - # Check the status of current stream and response. - re_use_res = False - try: - if self.stream is not None: - if self.stream.stage is Stage.IDLE: - raise ResponseException( - "Another response was sent previously." - ) - re_use_res = response is (self.stream and self.stream.response) - except AttributeError: - pass - # Connect the response if isinstance(response, BaseHTTPResponse) and self.stream: response = self.stream.respond(response) # Run response middleware - if not re_use_res: - try: - response = await self.app._run_response_middleware( - self, response, request_name=self.name - ) - except CancelledErrors: - raise - except Exception: - error_logger.exception( - "Exception occurred in one of response middleware handlers" - ) + try: + response = await self.app._run_response_middleware( + self, response, request_name=self.name + ) + except CancelledErrors: + raise + except Exception: + error_logger.exception( + "Exception occurred in one of response middleware handlers" + ) + self.responded = True return response async def receive_body(self): From 62c1dcbab717ce1d2b707d506cc9cf70ac7ad97d Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 2 Dec 2021 21:06:40 -0600 Subject: [PATCH 28/62] Try adding stage to asgi --- sanic/app.py | 2 +- sanic/asgi.py | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/sanic/app.py b/sanic/app.py index ea4271a4d1..a4eb94b2a6 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -737,7 +737,7 @@ async def handle_exception( ) if ( - isinstance(request.stream, Http) + request.stream is not None and request.stream.stage is not Stage.HANDLER ): error_logger.exception(exception) diff --git a/sanic/asgi.py b/sanic/asgi.py index 55c18d5cf5..1bb180ee82 100644 --- a/sanic/asgi.py +++ b/sanic/asgi.py @@ -7,6 +7,7 @@ from sanic.compat import Header from sanic.exceptions import ServerError +from sanic.http import Stage from sanic.models.asgi import ASGIReceive, ASGIScope, ASGISend, MockTransport from sanic.request import Request from sanic.server import ConnInfo @@ -83,6 +84,7 @@ class ASGIApp: transport: MockTransport lifespan: Lifespan ws: Optional[WebSocketConnection] + stage: Stage def __init__(self) -> None: self.ws = None @@ -95,6 +97,7 @@ async def create( instance.sanic_app = sanic_app instance.transport = MockTransport(scope, receive, send) instance.transport.loop = sanic_app.loop + instance.stage = Stage.IDLE setattr(instance.transport, "add_task", sanic_app.loop.create_task) headers = Header( @@ -149,6 +152,8 @@ async def read(self) -> Optional[bytes]: """ Read and stream the body in chunks from an incoming ASGI message. """ + if self.stage is Stage.IDLE: + self.stage = Stage.REQUEST message = await self.transport.receive() body = message.get("body", b"") if not message.get("more_body", False): @@ -164,10 +169,14 @@ async def __aiter__(self): yield data def respond(self, response): + if self.stage is not Stage.HANDLER: + self.stage = Stage.FAILED + raise RuntimeError("Response already started") response.stream, self.response = self, response return response async def send(self, data, end_stream): + self.stage = Stage.IDLE if end_stream else Stage.RESPONSE if self.response: response, self.response = self.response, None await self.transport.send( @@ -195,6 +204,8 @@ async def __call__(self) -> None: Handle the incoming request. """ try: + self.stage = Stage.HANDLER await self.sanic_app.handle_request(self.request) except Exception as e: + self.stage = Stage.FAILED await self.sanic_app.handle_exception(self.request, e) From 1ce2b443e10db6e3b110bef8c6c4b47edd61bd32 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 2 Dec 2021 21:07:29 -0600 Subject: [PATCH 29/62] Disconnect stream when new res generated (code by @Tronic) --- sanic/http.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sanic/http.py b/sanic/http.py index 6f59ef250f..86f23fe3e3 100644 --- a/sanic/http.py +++ b/sanic/http.py @@ -584,6 +584,11 @@ def respond(self, response: BaseHTTPResponse) -> BaseHTTPResponse: self.stage = Stage.FAILED raise RuntimeError("Response already started") + # Disconnect any earlier but unused response object + if self.response is not None: + self.response.stream = None + + # Connect and return the response self.response, response.stream = response, self return response From bcae910d09ed791a2b89e7d3dc4bc4f42c7d54e9 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 2 Dec 2021 21:07:55 -0600 Subject: [PATCH 30/62] Fix typing --- sanic/response.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sanic/response.py b/sanic/response.py index b092cee214..ee954ac5c3 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -46,7 +46,7 @@ def __init__(self): self.asgi: bool = False self.body: Optional[bytes] = None self.content_type: Optional[str] = None - self.stream: Http = None + self.stream: Optional[Http] = None self.status: int = None self.headers = Header({}) self._cookies: Optional[CookieJar] = None @@ -113,6 +113,10 @@ async def send( """ if data is None and end_stream is None: end_stream = True + if self.stream is None: + raise SanicException( + "Stream was disconnected with this response instance." + ) if self.stream.send is None: if end_stream and not data: return From c9eb6f0f775ef6ec73c90df419d86a6ffe16bf08 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 00:46:23 -0600 Subject: [PATCH 31/62] Fix --- sanic/asgi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanic/asgi.py b/sanic/asgi.py index 1bb180ee82..a47131a558 100644 --- a/sanic/asgi.py +++ b/sanic/asgi.py @@ -207,5 +207,5 @@ async def __call__(self) -> None: self.stage = Stage.HANDLER await self.sanic_app.handle_request(self.request) except Exception as e: - self.stage = Stage.FAILED await self.sanic_app.handle_exception(self.request, e) + self.stage = Stage.FAILED From d43f74b02635cd9d420934caa3a88955ba05cd64 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 00:53:32 -0600 Subject: [PATCH 32/62] Disconnect stream from response when resetting --- sanic/request.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sanic/request.py b/sanic/request.py index f5aac2c0aa..737baebe03 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -175,6 +175,7 @@ def reset_response(self): raise ResponseException( "Cannot reset response because previous response was sent." ) + self.stream.response.stream = None self.stream.response = None self.responded = False except AttributeError: From 7a2c684a05d19f3ce4bb078be6f3d830fdeeed56 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 01:30:09 -0600 Subject: [PATCH 33/62] Remove unnecessary test --- sanic/request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanic/request.py b/sanic/request.py index 737baebe03..029ab17334 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -196,7 +196,7 @@ async def respond( pass # This logic of determining which response to use is subject to change if response is None: - response = (self.stream and self.stream.response) or HTTPResponse( + response = HTTPResponse( status=status, headers=headers, content_type=content_type, From fbd4dfd2212910c3d97f7c50b2a29a44350fbb6d Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 01:32:44 -0600 Subject: [PATCH 34/62] Clear raising exception logic --- sanic/response.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sanic/response.py b/sanic/response.py index ee954ac5c3..928d128ec2 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -120,13 +120,11 @@ async def send( if self.stream.send is None: if end_stream and not data: return - elif self.stream.stage == Stage.IDLE: + if self.stream.stage is Stage.IDLE: raise ResponseException( "Response stream was ended, no more response data is " "allowed to be sent." ) - else: - raise SanicException("Send response function isn't available") data = ( data.encode() # type: ignore if hasattr(data, "encode") From 58b3613a12ed2fe3cdf5e1a7a2f28c6ce8de17f4 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 01:39:25 -0600 Subject: [PATCH 35/62] Fix lint --- sanic/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanic/app.py b/sanic/app.py index a4eb94b2a6..7d1f3c11db 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -66,7 +66,7 @@ URLBuildError, ) from sanic.handlers import ErrorHandler -from sanic.http import Http, Stage +from sanic.http import Stage from sanic.log import LOGGING_CONFIG_DEFAULTS, Colors, error_logger, logger from sanic.mixins.listeners import ListenerEvent from sanic.models.futures import ( From e4b53a01c2f633fbfea1a1d5b580fc457bf0cc1b Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 01:39:40 -0600 Subject: [PATCH 36/62] Fix typing check --- sanic/response.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sanic/response.py b/sanic/response.py index 928d128ec2..adae0d7076 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -121,10 +121,13 @@ async def send( if end_stream and not data: return if self.stream.stage is Stage.IDLE: - raise ResponseException( + msg = ( "Response stream was ended, no more response data is " "allowed to be sent." ) + else: + msg = "Send response function is not available." + raise ResponseException(msg) data = ( data.encode() # type: ignore if hasattr(data, "encode") From b4bd23a9198e202e1048d65a1d5f19254b4af9ee Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 02:18:46 -0600 Subject: [PATCH 37/62] Fix msg, replace ResponseException with ServerError; add exc_info=True to logger call --- sanic/app.py | 8 ++++---- sanic/exceptions.py | 4 ---- sanic/request.py | 6 +++--- sanic/response.py | 4 ++-- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 7d1f3c11db..11fa518f08 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -740,11 +740,11 @@ async def handle_exception( request.stream is not None and request.stream.stage is not Stage.HANDLER ): - error_logger.exception(exception) + error_logger.exception(exception, exc_info=True) logger.error( - "The error response won't be sent to the client for the " - f'exception: "{exception}" because a previous response has ' - "already been sent at least partially." + "The error response will not be sent to the client for " + f'the following exception:"{exception}". A previous response ' + "has at least partially been sent." ) return diff --git a/sanic/exceptions.py b/sanic/exceptions.py index f811326d7b..6459f15a42 100644 --- a/sanic/exceptions.py +++ b/sanic/exceptions.py @@ -266,7 +266,3 @@ def abort(status_code: int, message: Optional[Union[str, bytes]] = None): ) raise SanicException(message=message, status_code=status_code) - - -class ResponseException(SanicException): - pass diff --git a/sanic/request.py b/sanic/request.py index 029ab17334..ddec6e825d 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -31,7 +31,7 @@ from sanic.compat import CancelledErrors, Header from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE -from sanic.exceptions import InvalidUsage, ResponseException +from sanic.exceptions import InvalidUsage, ServerError from sanic.headers import ( AcceptContainer, Options, @@ -172,7 +172,7 @@ def reset_response(self): self.stream is not None and self.stream.stage is not Stage.HANDLER ): - raise ResponseException( + raise ServerError( "Cannot reset response because previous response was sent." ) self.stream.response.stream = None @@ -191,7 +191,7 @@ async def respond( ): try: if self.stream is not None and self.stream.response: - raise ResponseException("Second respond call is not allowed.") + raise ServerError("Second respond call is not allowed.") except AttributeError: pass # This logic of determining which response to use is subject to change diff --git a/sanic/response.py b/sanic/response.py index adae0d7076..94e0e5df9f 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -19,7 +19,7 @@ from sanic.compat import Header, open_async from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE from sanic.cookies import CookieJar -from sanic.exceptions import ResponseException, SanicException +from sanic.exceptions import SanicException, ServerError from sanic.helpers import has_message_body, remove_entity_headers from sanic.http import Http, Stage from sanic.models.protocol_types import HTMLProtocol, Range @@ -127,7 +127,7 @@ async def send( ) else: msg = "Send response function is not available." - raise ResponseException(msg) + raise ServerError(msg) data = ( data.encode() # type: ignore if hasattr(data, "encode") From 214c1eadccaf437bf6ce8480499eb96d2f44ffc8 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 17:18:31 -0600 Subject: [PATCH 38/62] Simplefied error raising logic --- sanic/response.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/sanic/response.py b/sanic/response.py index 94e0e5df9f..d644e118d9 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -120,14 +120,10 @@ async def send( if self.stream.send is None: if end_stream and not data: return - if self.stream.stage is Stage.IDLE: - msg = ( - "Response stream was ended, no more response data is " - "allowed to be sent." - ) - else: - msg = "Send response function is not available." - raise ServerError(msg) + raise ServerError( + "Response stream was ended, no more response data is " + "allowed to be sent." + ) data = ( data.encode() # type: ignore if hasattr(data, "encode") From 32b6b98d118f94d69a93d28a577b231385d8c14d Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 17:22:43 -0600 Subject: [PATCH 39/62] Disconnect prev response in agi.respond --- sanic/asgi.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sanic/asgi.py b/sanic/asgi.py index a47131a558..75aa832600 100644 --- a/sanic/asgi.py +++ b/sanic/asgi.py @@ -172,6 +172,8 @@ def respond(self, response): if self.stage is not Stage.HANDLER: self.stage = Stage.FAILED raise RuntimeError("Response already started") + if self.response is not None: + self.response.stream = None response.stream, self.response = self, response return response From 69e8c114b420b1f153c8ca04de5f6036c8e6c346 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 17:45:49 -0600 Subject: [PATCH 40/62] Add response attr to ASGIApp, fix typing --- sanic/asgi.py | 5 ++++- sanic/response.py | 8 ++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/sanic/asgi.py b/sanic/asgi.py index 75aa832600..7b31cccb38 100644 --- a/sanic/asgi.py +++ b/sanic/asgi.py @@ -10,6 +10,7 @@ from sanic.http import Stage from sanic.models.asgi import ASGIReceive, ASGIScope, ASGISend, MockTransport from sanic.request import Request +from sanic.response import BaseHTTPResponse from sanic.server import ConnInfo from sanic.server.websockets.connection import WebSocketConnection @@ -85,6 +86,7 @@ class ASGIApp: lifespan: Lifespan ws: Optional[WebSocketConnection] stage: Stage + response: Optional[BaseHTTPResponse] def __init__(self) -> None: self.ws = None @@ -98,6 +100,7 @@ async def create( instance.transport = MockTransport(scope, receive, send) instance.transport.loop = sanic_app.loop instance.stage = Stage.IDLE + instance.response = None setattr(instance.transport, "add_task", sanic_app.loop.create_task) headers = Header( @@ -168,7 +171,7 @@ async def __aiter__(self): if data: yield data - def respond(self, response): + def respond(self, response: BaseHTTPResponse): if self.stage is not Stage.HANDLER: self.stage = Stage.FAILED raise RuntimeError("Response already started") diff --git a/sanic/response.py b/sanic/response.py index d644e118d9..dceb52120e 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -3,6 +3,7 @@ from os import path from pathlib import PurePath from typing import ( + TYPE_CHECKING, Any, AnyStr, Callable, @@ -21,10 +22,13 @@ from sanic.cookies import CookieJar from sanic.exceptions import SanicException, ServerError from sanic.helpers import has_message_body, remove_entity_headers -from sanic.http import Http, Stage +from sanic.http import Http from sanic.models.protocol_types import HTMLProtocol, Range +if TYPE_CHECKING: + from sanic.asgi import ASGIApp + try: from ujson import dumps as json_dumps except ImportError: @@ -46,7 +50,7 @@ def __init__(self): self.asgi: bool = False self.body: Optional[bytes] = None self.content_type: Optional[str] = None - self.stream: Optional[Http] = None + self.stream: Optional[Union[Http, ASGIApp]] = None self.status: int = None self.headers = Header({}) self._cookies: Optional[CookieJar] = None From 97b075d6602fe6edfefa474b1b132933d15e853e Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Fri, 3 Dec 2021 20:49:11 -0600 Subject: [PATCH 41/62] Strean finally be in Stage.IDLE --- sanic/asgi.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sanic/asgi.py b/sanic/asgi.py index 7b31cccb38..ba2fae8daa 100644 --- a/sanic/asgi.py +++ b/sanic/asgi.py @@ -213,4 +213,5 @@ async def __call__(self) -> None: await self.sanic_app.handle_request(self.request) except Exception as e: await self.sanic_app.handle_exception(self.request, e) - self.stage = Stage.FAILED + finally: + self.stage = Stage.IDLE From 8996167d3109610e2ed567fe59524a7fb1497913 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Sun, 5 Dec 2021 03:00:06 -0600 Subject: [PATCH 42/62] Only set stage to idle when it is in response --- sanic/asgi.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sanic/asgi.py b/sanic/asgi.py index ba2fae8daa..56de38d490 100644 --- a/sanic/asgi.py +++ b/sanic/asgi.py @@ -214,4 +214,5 @@ async def __call__(self) -> None: except Exception as e: await self.sanic_app.handle_exception(self.request, e) finally: - self.stage = Stage.IDLE + if self.stage == Stage.RESPONSE: + self.stage = Stage.IDLE From 8e67b11617b1bfa02eda169766dd88e5591dac9c Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Sun, 5 Dec 2021 03:11:58 -0600 Subject: [PATCH 43/62] Match http behavior --- sanic/asgi.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/sanic/asgi.py b/sanic/asgi.py index 56de38d490..00b181dcde 100644 --- a/sanic/asgi.py +++ b/sanic/asgi.py @@ -213,6 +213,3 @@ async def __call__(self) -> None: await self.sanic_app.handle_request(self.request) except Exception as e: await self.sanic_app.handle_exception(self.request, e) - finally: - if self.stage == Stage.RESPONSE: - self.stage = Stage.IDLE From 61d56ee29cc857b9aa1328a1cd803bf37917c7d0 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Tue, 7 Dec 2021 03:38:32 -0600 Subject: [PATCH 44/62] Add the deprecated warning for exception handler --- sanic/app.py | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index f0ef124fec..51b8b60f97 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -725,9 +725,7 @@ async def handle_exception( A handler that catches specific exceptions and outputs a response. :param request: The current request object - :type request: :class:`SanicASGITestClient` :param exception: The exception that was raised - :type exception: BaseException :raises ServerError: response 500 """ await self.dispatch( @@ -746,6 +744,34 @@ async def handle_exception( f'the following exception:"{exception}". A previous response ' "has at least partially been sent." ) + + # ----------------- deprecated ----------------- + if self.error_handler._lookup( + exception, request.name if request else None + ): + logger.warning( + "An error occurs during the response process, " + "Sanic no longer execute that exception handler " + "beginning in v22.6 because the error page generated " + "by the exception handler won't be sent to the client. " + "The exception handler should only be used to generate " + "the error page, and if you would like to perform any " + "other action for an exception raised, please consider " + "using a signal handler, like " + '`@app.signal("http.lifecycle.exception")`\n' + "Sanic signals detailed docs: " + "https://sanicframework.org/en/guide/advanced/" + "signals.html" + ) + try: + response = self.error_handler.response(request, exception) + if isawaitable(response): + response = await response + except BaseException as e: + logger.error("An error occurs in the exception handler.") + error_logger.exception(e) + # ---------------------------------------------- + return # -------------------------------------------- # From 74a43771d42b220cef1cb145fa52351ee79edfbc Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Tue, 7 Dec 2021 04:12:23 -0600 Subject: [PATCH 45/62] Log an error if a res returned by a route handler when a pre res was sent --- sanic/app.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 51b8b60f97..b5cb3599fe 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -913,8 +913,15 @@ async def handle_request(self, request: Request): # no cov if isawaitable(response): response = await response - if response is not None and not request.responded: - response = await request.respond(response) + if response is not None: + if not request.responded: + response = await request.respond(response) + else: + error_logger.error( + "The response object returned by the route handler " + "won't be sent to client because a previous response " + "was created and may have been sent the client." + ) elif not hasattr(handler, "is_websocket"): response = request.stream.response # type: ignore From 6f5d3786ee5e8fbb3f4825ebecfef6f54536eb3f Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Tue, 7 Dec 2021 21:46:34 -0600 Subject: [PATCH 46/62] Add a test case and type hints for tests --- tests/test_exceptions_handler.py | 79 ++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 13 deletions(-) diff --git a/tests/test_exceptions_handler.py b/tests/test_exceptions_handler.py index 9ad595fc18..57dff53cdd 100644 --- a/tests/test_exceptions_handler.py +++ b/tests/test_exceptions_handler.py @@ -6,10 +6,18 @@ import pytest from bs4 import BeautifulSoup +from pytest import LogCaptureFixture, MonkeyPatch from sanic import Sanic, handlers -from sanic.exceptions import Forbidden, InvalidUsage, NotFound, ServerError +from sanic.exceptions import ( + Forbidden, + InvalidUsage, + NotFound, + SanicException, + ServerError, +) from sanic.handlers import ErrorHandler +from sanic.request import Request from sanic.response import stream, text @@ -90,35 +98,35 @@ async def some_request_middleware(request): return exception_handler_app -def test_invalid_usage_exception_handler(exception_handler_app): +def test_invalid_usage_exception_handler(exception_handler_app: Sanic): request, response = exception_handler_app.test_client.get("/1") assert response.status == 400 -def test_server_error_exception_handler(exception_handler_app): +def test_server_error_exception_handler(exception_handler_app: Sanic): request, response = exception_handler_app.test_client.get("/2") assert response.status == 200 assert response.text == "OK" -def test_not_found_exception_handler(exception_handler_app): +def test_not_found_exception_handler(exception_handler_app: Sanic): request, response = exception_handler_app.test_client.get("/3") assert response.status == 200 -def test_text_exception__handler(exception_handler_app): +def test_text_exception__handler(exception_handler_app: Sanic): request, response = exception_handler_app.test_client.get("/random") assert response.status == 200 assert response.text == "Done." -def test_async_exception_handler(exception_handler_app): +def test_async_exception_handler(exception_handler_app: Sanic): request, response = exception_handler_app.test_client.get("/7") assert response.status == 200 assert response.text == "foo,bar" -def test_html_traceback_output_in_debug_mode(exception_handler_app): +def test_html_traceback_output_in_debug_mode(exception_handler_app: Sanic): request, response = exception_handler_app.test_client.get("/4", debug=True) assert response.status == 500 soup = BeautifulSoup(response.body, "html.parser") @@ -133,12 +141,12 @@ def test_html_traceback_output_in_debug_mode(exception_handler_app): ) == summary_text -def test_inherited_exception_handler(exception_handler_app): +def test_inherited_exception_handler(exception_handler_app: Sanic): request, response = exception_handler_app.test_client.get("/5") assert response.status == 200 -def test_chained_exception_handler(exception_handler_app): +def test_chained_exception_handler(exception_handler_app: Sanic): request, response = exception_handler_app.test_client.get( "/6/0", debug=True ) @@ -157,7 +165,7 @@ def test_chained_exception_handler(exception_handler_app): ) == summary_text -def test_exception_handler_lookup(exception_handler_app): +def test_exception_handler_lookup(exception_handler_app: Sanic): class CustomError(Exception): pass @@ -205,13 +213,17 @@ class ModuleNotFoundError(ImportError): ) -def test_exception_handler_processed_request_middleware(exception_handler_app): +def test_exception_handler_processed_request_middleware( + exception_handler_app: Sanic, +): request, response = exception_handler_app.test_client.get("/8") assert response.status == 200 assert response.text == "Done." -def test_single_arg_exception_handler_notice(exception_handler_app, caplog): +def test_single_arg_exception_handler_notice( + exception_handler_app: Sanic, caplog: LogCaptureFixture +): class CustomErrorHandler(ErrorHandler): def lookup(self, exception): return super().lookup(exception, None) @@ -235,7 +247,9 @@ def lookup(self, exception): assert response.status == 400 -def test_error_handler_noisy_log(exception_handler_app, monkeypatch): +def test_error_handler_noisy_log( + exception_handler_app: Sanic, monkeypatch: MonkeyPatch +): err_logger = Mock() monkeypatch.setattr(handlers, "error_logger", err_logger) @@ -248,3 +262,42 @@ def test_error_handler_noisy_log(exception_handler_app, monkeypatch): err_logger.exception.assert_called_with( "Exception occurred while handling uri: %s", repr(request.url) ) + + +def test_exception_handler_response_was_sent( + app: Sanic, caplog: LogCaptureFixture +): + exception_handler_ran = False + + @app.exception(SanicException) + def exception_handler(request, exception): + nonlocal exception_handler_ran + exception_handler_ran = True + return text("OK") + + @app.route("/") + async def handler(request: Request): + response = await request.respond() + await response.send("some text") + raise SanicException("Exception") + + with caplog.at_level(logging.WARNING): + _, response = app.test_client.get("/") + assert "some text" in response.text + + depreciated_warning_issued = False + for record in caplog.records: + if record.message.startswith( + "An error occurs during the response process, " + "Sanic no longer execute that exception handler " + "beginning in v22.6 because the error page generated " + "by the exception handler won't be sent to the client " + ): + depreciated_warning_issued = True + + assert depreciated_warning_issued + assert exception_handler_ran + + # In the future version: + # assert not exception_handler_ran + # removing asserting for the depreciated-warning From cab3f6861a9c57fc25bc54af50629f293b8676a5 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Tue, 7 Dec 2021 22:14:18 -0600 Subject: [PATCH 47/62] Update error log messages --- sanic/app.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index b5cb3599fe..27614d0ea2 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -753,7 +753,8 @@ async def handle_exception( "An error occurs during the response process, " "Sanic no longer execute that exception handler " "beginning in v22.6 because the error page generated " - "by the exception handler won't be sent to the client. " + "by the exception handler won't be sent to the client " + "because another response was at least partially sent " "The exception handler should only be used to generate " "the error page, and if you would like to perform any " "other action for an exception raised, please consider " @@ -920,7 +921,8 @@ async def handle_request(self, request: Request): # no cov error_logger.error( "The response object returned by the route handler " "won't be sent to client because a previous response " - "was created and may have been sent the client." + "or itself was created and may have been sent the " + "client. " ) elif not hasattr(handler, "is_websocket"): response = request.stream.response # type: ignore From 71e69ab4de0551c819c5baf953245d2ddf8d9212 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 00:48:01 -0600 Subject: [PATCH 48/62] Update and add test cases --- tests/conftest.py | 16 ++- tests/test_exceptions_handler.py | 13 +-- tests/test_response.py | 174 +++++++++++++++++++++++++------ 3 files changed, 164 insertions(+), 39 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 175e967efa..292914cd4d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,7 +6,8 @@ import sys import uuid -from typing import Tuple +from logging import LogRecord +from typing import Callable, List, Tuple import pytest @@ -170,3 +171,16 @@ def run(app): return caplog.record_tuples return run + + +@pytest.fixture(scope="function") +def message_in_records(): + def msg_in_log(records: List[LogRecord], msg: str): + error_captured = False + for record in records: + if msg in record.message: + error_captured = True + break + return error_captured + + return msg_in_log diff --git a/tests/test_exceptions_handler.py b/tests/test_exceptions_handler.py index 57dff53cdd..62a44e20ec 100644 --- a/tests/test_exceptions_handler.py +++ b/tests/test_exceptions_handler.py @@ -9,13 +9,7 @@ from pytest import LogCaptureFixture, MonkeyPatch from sanic import Sanic, handlers -from sanic.exceptions import ( - Forbidden, - InvalidUsage, - NotFound, - SanicException, - ServerError, -) +from sanic.exceptions import Forbidden, InvalidUsage, NotFound, ServerError from sanic.handlers import ErrorHandler from sanic.request import Request from sanic.response import stream, text @@ -269,7 +263,7 @@ def test_exception_handler_response_was_sent( ): exception_handler_ran = False - @app.exception(SanicException) + @app.exception(ServerError) def exception_handler(request, exception): nonlocal exception_handler_ran exception_handler_ran = True @@ -279,7 +273,7 @@ def exception_handler(request, exception): async def handler(request: Request): response = await request.respond() await response.send("some text") - raise SanicException("Exception") + raise ServerError("Exception") with caplog.at_level(logging.WARNING): _, response = app.test_client.get("/") @@ -294,6 +288,7 @@ async def handler(request: Request): "by the exception handler won't be sent to the client " ): depreciated_warning_issued = True + break assert depreciated_warning_issued assert exception_handler_ran diff --git a/tests/test_response.py b/tests/test_response.py index 0676b8851f..fb43b8fac3 100644 --- a/tests/test_response.py +++ b/tests/test_response.py @@ -3,15 +3,18 @@ import os from collections import namedtuple +from logging import ERROR, LogRecord from mimetypes import guess_type from random import choice +from typing import Callable, List from urllib.parse import unquote import pytest from aiofiles import os as async_os +from pytest import LogCaptureFixture -from sanic import Sanic +from sanic import Request, Sanic from sanic.response import ( HTTPResponse, empty, @@ -33,7 +36,7 @@ def test_response_body_not_a_string(app): random_num = choice(range(1000)) @app.route("/hello") - async def hello_route(request): + async def hello_route(request: Request): return text(random_num) request, response = app.test_client.get("/hello") @@ -51,7 +54,7 @@ def test_method_not_allowed(): app = Sanic("app") @app.get("/") - async def test_get(request): + async def test_get(request: Request): return response.json({"hello": "world"}) request, response = app.test_client.head("/") @@ -67,7 +70,7 @@ async def test_get(request): app.router.reset() @app.post("/") - async def test_post(request): + async def test_post(request: Request): return response.json({"hello": "world"}) request, response = app.test_client.head("/") @@ -89,7 +92,7 @@ async def test_post(request): def test_response_header(app): @app.get("/") - async def test(request): + async def test(request: Request): return json({"ok": True}, headers={"CONTENT-TYPE": "application/json"}) request, response = app.test_client.get("/") @@ -102,14 +105,14 @@ async def test(request): def test_response_content_length(app): @app.get("/response_with_space") - async def response_with_space(request): + async def response_with_space(request: Request): return json( {"message": "Data", "details": "Some Details"}, headers={"CONTENT-TYPE": "application/json"}, ) @app.get("/response_without_space") - async def response_without_space(request): + async def response_without_space(request: Request): return json( {"message": "Data", "details": "Some Details"}, headers={"CONTENT-TYPE": "application/json"}, @@ -135,7 +138,7 @@ async def response_without_space(request): def test_response_content_length_with_different_data_types(app): @app.get("/") - async def get_data_with_different_types(request): + async def get_data_with_different_types(request: Request): # Indentation issues in the Response is intentional. Please do not fix return json( {"bool": True, "none": None, "string": "string", "number": -1}, @@ -149,23 +152,23 @@ async def get_data_with_different_types(request): @pytest.fixture def json_app(app): @app.route("/") - async def test(request): + async def test(request: Request): return json(JSON_DATA) @app.get("/no-content") - async def no_content_handler(request): + async def no_content_handler(request: Request): return json(JSON_DATA, status=204) @app.get("/no-content/unmodified") - async def no_content_unmodified_handler(request): + async def no_content_unmodified_handler(request: Request): return json(None, status=304) @app.get("/unmodified") - async def unmodified_handler(request): + async def unmodified_handler(request: Request): return json(JSON_DATA, status=304) @app.delete("/") - async def delete_handler(request): + async def delete_handler(request: Request): return json(None, status=204) return app @@ -207,7 +210,7 @@ def test_no_content(json_app): @pytest.fixture def streaming_app(app): @app.route("/") - async def test(request): + async def test(request: Request): return stream( sample_streaming_fn, content_type="text/csv", @@ -219,7 +222,7 @@ async def test(request): @pytest.fixture def non_chunked_streaming_app(app): @app.route("/") - async def test(request): + async def test(request: Request): return stream( sample_streaming_fn, headers={"Content-Length": "7"}, @@ -276,7 +279,7 @@ def test_non_chunked_streaming_returns_correct_content( def test_stream_response_with_cookies(app): @app.route("/") - async def test(request): + async def test(request: Request): response = stream(sample_streaming_fn, content_type="text/csv") response.cookies["test"] = "modified" response.cookies["test"] = "pass" @@ -288,7 +291,7 @@ async def test(request): def test_stream_response_without_cookies(app): @app.route("/") - async def test(request): + async def test(request: Request): return stream(sample_streaming_fn, content_type="text/csv") request, response = app.test_client.get("/") @@ -314,7 +317,7 @@ def get_file_content(static_file_directory, file_name): "file_name", ["test.file", "decode me.txt", "python.png"] ) @pytest.mark.parametrize("status", [200, 401]) -def test_file_response(app, file_name, static_file_directory, status): +def test_file_response(app: Sanic, file_name, static_file_directory, status): @app.route("/files/", methods=["GET"]) def file_route(request, filename): file_path = os.path.join(static_file_directory, filename) @@ -340,7 +343,7 @@ def file_route(request, filename): ], ) def test_file_response_custom_filename( - app, source, dest, static_file_directory + app: Sanic, source, dest, static_file_directory ): @app.route("/files/", methods=["GET"]) def file_route(request, filename): @@ -358,7 +361,7 @@ def file_route(request, filename): @pytest.mark.parametrize("file_name", ["test.file", "decode me.txt"]) -def test_file_head_response(app, file_name, static_file_directory): +def test_file_head_response(app: Sanic, file_name, static_file_directory): @app.route("/files/", methods=["GET", "HEAD"]) async def file_route(request, filename): file_path = os.path.join(static_file_directory, filename) @@ -391,7 +394,7 @@ async def file_route(request, filename): @pytest.mark.parametrize( "file_name", ["test.file", "decode me.txt", "python.png"] ) -def test_file_stream_response(app, file_name, static_file_directory): +def test_file_stream_response(app: Sanic, file_name, static_file_directory): @app.route("/files/", methods=["GET"]) def file_route(request, filename): file_path = os.path.join(static_file_directory, filename) @@ -417,7 +420,7 @@ def file_route(request, filename): ], ) def test_file_stream_response_custom_filename( - app, source, dest, static_file_directory + app: Sanic, source, dest, static_file_directory ): @app.route("/files/", methods=["GET"]) def file_route(request, filename): @@ -435,7 +438,9 @@ def file_route(request, filename): @pytest.mark.parametrize("file_name", ["test.file", "decode me.txt"]) -def test_file_stream_head_response(app, file_name, static_file_directory): +def test_file_stream_head_response( + app: Sanic, file_name, static_file_directory +): @app.route("/files/", methods=["GET", "HEAD"]) async def file_route(request, filename): file_path = os.path.join(static_file_directory, filename) @@ -479,7 +484,7 @@ async def file_route(request, filename): "size,start,end", [(1024, 0, 1024), (4096, 1024, 8192)] ) def test_file_stream_response_range( - app, file_name, static_file_directory, size, start, end + app: Sanic, file_name, static_file_directory, size, start, end ): Range = namedtuple("Range", ["size", "start", "end", "total"]) @@ -508,7 +513,7 @@ def file_route(request, filename): def test_raw_response(app): @app.get("/test") - def handler(request): + def handler(request: Request): return raw(b"raw_response") request, response = app.test_client.get("/test") @@ -518,7 +523,7 @@ def handler(request): def test_empty_response(app): @app.get("/test") - def handler(request): + def handler(request: Request): return empty() request, response = app.test_client.get("/test") @@ -526,17 +531,128 @@ def handler(request): assert response.body == b"" -def test_direct_response_stream(app): +def test_direct_response_stream(app: Sanic): @app.route("/") - async def test(request): + async def test(request: Request): response = await request.respond(content_type="text/csv") await response.send("foo,") await response.send("bar") await response.eof() - return response _, response = app.test_client.get("/") assert response.text == "foo,bar" assert response.headers["Transfer-Encoding"] == "chunked" assert response.headers["Content-Type"] == "text/csv" assert "Content-Length" not in response.headers + + +def test_two_respond_calls(app: Sanic): + @app.route("/") + async def handler(request: Request): + response = await request.respond() + await response.send("foo,") + await response.send("bar") + await response.eof() + + +def test_multiple_responses( + app: Sanic, + caplog: LogCaptureFixture, + message_in_records: Callable[[List[LogRecord], str], bool], +): + @app.route("/1") + async def handler(request: Request): + response = await request.respond() + await response.send("foo") + response = await request.respond() + + @app.route("/2") + async def handler(request: Request): + response = await request.respond() + response = await request.respond() + await response.send("foo") + + @app.get("/3") + async def handler(request: Request): + response = await request.respond() + await response.send("foo,") + response = await request.respond() + await response.send("bar") + + @app.get("/4") + async def handler(request: Request): + response = await request.respond(headers={"one": "one"}) + return json({"foo": "bar"}, headers={"one": "two"}) + + @app.get("/5") + async def handler(request: Request): + response = await request.respond(headers={"one": "one"}) + await response.send("foo") + return json({"foo": "bar"}, headers={"one": "two"}) + + @app.get("/6") + async def handler(request: Request): + response = await request.respond(headers={"one": "one"}) + await response.send("foo, ") + json_response = json({"foo": "bar"}, headers={"one": "two"}) + await response.send("bar") + return json_response + + error_msg0 = "Second respond call is not allowed." + + error_msg1 = ( + "The error response will not be sent to the client for the following " + 'exception:"Second respond call is not allowed.". A previous ' + "response has at least partially been sent." + ) + + error_msg2 = ( + "The response object returned by the route handler " + "won't be sent to client because a previous response " + "or itself was created and may have been sent the " + "client. " + ) + + with caplog.at_level(ERROR): + _, response = app.test_client.get("/1") + assert response.status == 200 + assert message_in_records(caplog.records, error_msg0) + assert message_in_records(caplog.records, error_msg1) + + with caplog.at_level(ERROR): + _, response = app.test_client.get("/2") + assert response.status == 500 + assert "500 — Internal Server Error" in response.text + + with caplog.at_level(ERROR): + _, response = app.test_client.get("/3") + assert response.status == 200 + assert "foo," in response.text + assert message_in_records(caplog.records, error_msg0) + assert message_in_records(caplog.records, error_msg1) + + with caplog.at_level(ERROR): + _, response = app.test_client.get("/4") + print(response.json) + assert response.status == 200 + assert "foo" not in response.text + assert "one" in response.headers + assert response.headers["one"] == "one" + + print(response.headers) + assert message_in_records(caplog.records, error_msg2) + + with caplog.at_level(ERROR): + _, response = app.test_client.get("/5") + assert response.status == 200 + assert "foo" in response.text + assert "one" in response.headers + assert response.headers["one"] == "one" + assert message_in_records(caplog.records, error_msg2) + + with caplog.at_level(ERROR): + _, response = app.test_client.get("/6") + assert "foo, bar" in response.text + assert "one" in response.headers + assert response.headers["one"] == "one" + assert message_in_records(caplog.records, error_msg2) From 8be48a2ab42bd2e4882708a754357f4e34e66dc6 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 00:48:52 -0600 Subject: [PATCH 49/62] Fix app --- sanic/app.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sanic/app.py b/sanic/app.py index 27614d0ea2..8d32d4452e 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -918,6 +918,7 @@ async def handle_request(self, request: Request): # no cov if not request.responded: response = await request.respond(response) else: + response = request.stream.response error_logger.error( "The response object returned by the route handler " "won't be sent to client because a previous response " From 19717fcf8c13cf6844f0db5f074a54123111e797 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 01:06:14 -0600 Subject: [PATCH 50/62] Fix typing and better if statement logic --- sanic/app.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 8d32d4452e..2224b984ea 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -914,19 +914,18 @@ async def handle_request(self, request: Request): # no cov if isawaitable(response): response = await response - if response is not None: - if not request.responded: - response = await request.respond(response) - else: + if response is not None and not request.responded: + response = await request.respond(response) + elif not hasattr(handler, "is_websocket") or request.responded: + if request.stream is not None: response = request.stream.response + if request.responded: error_logger.error( "The response object returned by the route handler " "won't be sent to client because a previous response " "or itself was created and may have been sent the " "client. " ) - elif not hasattr(handler, "is_websocket"): - response = request.stream.response # type: ignore # Make sure that response is finished / run StreamingHTTP callback if isinstance(response, BaseHTTPResponse): From cb3abfebe39e6b65d33b3248cf474ccd05aca3d7 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 01:29:51 -0600 Subject: [PATCH 51/62] Add test cases for sending data after eof --- tests/test_response.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_response.py b/tests/test_response.py index fb43b8fac3..4943b77aed 100644 --- a/tests/test_response.py +++ b/tests/test_response.py @@ -598,6 +598,13 @@ async def handler(request: Request): await response.send("bar") return json_response + @app.get("/7") + async def handler(request: Request): + response = await request.respond() + await response.send("foo, ") + await response.eof() + await response.send("bar") + error_msg0 = "Second respond call is not allowed." error_msg1 = ( @@ -613,6 +620,11 @@ async def handler(request: Request): "client. " ) + error_msg3 = ( + "Response stream was ended, no more " + "response data is allowed to be sent." + ) + with caplog.at_level(ERROR): _, response = app.test_client.get("/1") assert response.status == 200 @@ -656,3 +668,9 @@ async def handler(request: Request): assert "one" in response.headers assert response.headers["one"] == "one" assert message_in_records(caplog.records, error_msg2) + + with caplog.at_level(ERROR): + _, response = app.test_client.get("/7") + assert "foo, " in response.text + assert message_in_records(caplog.records, error_msg1) + assert message_in_records(caplog.records, error_msg3) From 64a12a9b1d419f54c0eb7af1c3b4da19342fcb76 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 01:43:04 -0600 Subject: [PATCH 52/62] Move send after eof case out and tobe a independent case --- tests/test_response.py | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/tests/test_response.py b/tests/test_response.py index 4943b77aed..a45de75f62 100644 --- a/tests/test_response.py +++ b/tests/test_response.py @@ -598,13 +598,6 @@ async def handler(request: Request): await response.send("bar") return json_response - @app.get("/7") - async def handler(request: Request): - response = await request.respond() - await response.send("foo, ") - await response.eof() - await response.send("bar") - error_msg0 = "Second respond call is not allowed." error_msg1 = ( @@ -669,8 +662,32 @@ async def handler(request: Request): assert response.headers["one"] == "one" assert message_in_records(caplog.records, error_msg2) + +def send_response_after_eof_should_fail( + app: Sanic, + caplog: LogCaptureFixture, + message_in_records: Callable[[List[LogRecord], str], bool], +): + @app.get("/") + async def handler(request: Request): + response = await request.respond() + await response.send("foo, ") + await response.eof() + await response.send("bar") + + error_msg1 = ( + "The error response will not be sent to the client for the following " + 'exception:"Second respond call is not allowed.". A previous ' + "response has at least partially been sent." + ) + + error_msg2 = ( + "Response stream was ended, no more " + "response data is allowed to be sent." + ) + with caplog.at_level(ERROR): - _, response = app.test_client.get("/7") + _, response = app.test_client.get("/") assert "foo, " in response.text assert message_in_records(caplog.records, error_msg1) - assert message_in_records(caplog.records, error_msg3) + assert message_in_records(caplog.records, error_msg2) From 37343b24e6baa173e88c161906adc9244789e83d Mon Sep 17 00:00:00 2001 From: Zhiwei <43905414+ChihweiLHBird@users.noreply.github.com> Date: Wed, 8 Dec 2021 02:23:24 -0700 Subject: [PATCH 53/62] Update error msg Co-authored-by: Adam Hopkins --- sanic/app.py | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 2224b984ea..eb7c5b3a20 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -746,23 +746,24 @@ async def handle_exception( ) # ----------------- deprecated ----------------- - if self.error_handler._lookup( + handler = self.error_handler._lookup( exception, request.name if request else None - ): - logger.warning( - "An error occurs during the response process, " - "Sanic no longer execute that exception handler " - "beginning in v22.6 because the error page generated " - "by the exception handler won't be sent to the client " - "because another response was at least partially sent " - "The exception handler should only be used to generate " - "the error page, and if you would like to perform any " - "other action for an exception raised, please consider " - "using a signal handler, like " - '`@app.signal("http.lifecycle.exception")`\n' - "Sanic signals detailed docs: " - "https://sanicframework.org/en/guide/advanced/" - "signals.html" + ) + if handler: + error_logger.warning( + "An error occured while handling the request after at ", + "least some part of the response was sent to the client. ", + "Therefore, the response from your custom exception ", + f"handler {handler.__name__} will not be sent to the ", + "client. Beginning in v22.6, Sanic will stop executing ", + "custom exception handlers in this scenario. Exception ", + "handlers should only be used to generate the exception ", + "responses. If you would like to perform any other action ", + "on a raised exception, please consider using a signal ", + 'handler like `@app.signal("http.lifecycle.exception")`\n', + "For further information, please see the docs: ", + "https://sanicframework.org/en/guide/advanced/", + "signals.html", ) try: response = self.error_handler.response(request, exception) From d391893da3cab3b60cc71260af55e2c8699b599e Mon Sep 17 00:00:00 2001 From: Zhiwei <43905414+ChihweiLHBird@users.noreply.github.com> Date: Wed, 8 Dec 2021 02:23:44 -0700 Subject: [PATCH 54/62] Update error msg2 Co-authored-by: Adam Hopkins --- sanic/app.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index eb7c5b3a20..51f549f5eb 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -923,9 +923,8 @@ async def handle_request(self, request: Request): # no cov if request.responded: error_logger.error( "The response object returned by the route handler " - "won't be sent to client because a previous response " - "or itself was created and may have been sent the " - "client. " + "will not be sent to client. The request has already " + "been responded to." ) # Make sure that response is finished / run StreamingHTTP callback From a35054715f4a95678d3f73850b623dcb2ed9006c Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 03:25:39 -0600 Subject: [PATCH 55/62] Update error msg3 --- sanic/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanic/app.py b/sanic/app.py index 2224b984ea..0d1773b4f5 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -769,7 +769,7 @@ async def handle_exception( if isawaitable(response): response = await response except BaseException as e: - logger.error("An error occurs in the exception handler.") + logger.error("An error occurred in the exception handler.") error_logger.exception(e) # ---------------------------------------------- From bdd8210c9c013f70717d359ccd2afc65a5c7587c Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 13:21:56 -0600 Subject: [PATCH 56/62] Update error msg --- sanic/response.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanic/response.py b/sanic/response.py index dceb52120e..357668e682 100644 --- a/sanic/response.py +++ b/sanic/response.py @@ -119,7 +119,7 @@ async def send( end_stream = True if self.stream is None: raise SanicException( - "Stream was disconnected with this response instance." + "No stream is connected to the response object instance." ) if self.stream.send is None: if end_stream and not data: From 0ad8f52f0fa78ca70285c8ed43abbce903b8427e Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 13:30:06 -0600 Subject: [PATCH 57/62] Fix lint --- sanic/app.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 18a53c0dee..46667ff8ff 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -761,9 +761,10 @@ async def handle_exception( "client. Beginning in v22.6, Sanic will stop executing ", "custom exception handlers in this scenario. Exception ", "handlers should only be used to generate the exception ", - "responses. If you would like to perform any other action ", - "on a raised exception, please consider using a signal ", - 'handler like `@app.signal("http.lifecycle.exception")`\n', + "responses. If you would like to perform any other ", + "action on a raised exception, please consider using a ", + "signal handler like ", + '`@app.signal("http.lifecycle.exception")`\n', "For further information, please see the docs: ", "https://sanicframework.org/en/guide/advanced/", "signals.html", From 61d8ead3b352732ed656c972a79c5ee68721fdab Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 14:32:11 -0600 Subject: [PATCH 58/62] Update error msgs in tests --- tests/test_exceptions_handler.py | 7 +++---- tests/test_response.py | 5 ++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/test_exceptions_handler.py b/tests/test_exceptions_handler.py index 082fb35d52..6e763c8105 100644 --- a/tests/test_exceptions_handler.py +++ b/tests/test_exceptions_handler.py @@ -280,10 +280,9 @@ async def handler(request: Request): depreciated_warning_issued = False for record in caplog.records: if record.message.startswith( - "An error occurs during the response process, " - "Sanic no longer execute that exception handler " - "beginning in v22.6 because the error page generated " - "by the exception handler won't be sent to the client " + "An error occurred while handling the request after at ", + "least some part of the response was sent to the client. ", + "Therefore, the response from your custom exception ", ): depreciated_warning_issued = True break diff --git a/tests/test_response.py b/tests/test_response.py index a45de75f62..8d301abfee 100644 --- a/tests/test_response.py +++ b/tests/test_response.py @@ -608,9 +608,8 @@ async def handler(request: Request): error_msg2 = ( "The response object returned by the route handler " - "won't be sent to client because a previous response " - "or itself was created and may have been sent the " - "client. " + "will not be sent to client. The request has already " + "been responded to." ) error_msg3 = ( From c8f2c3e2540c9906b9a1d092598c06d4d7a8bc2d Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 18:29:50 -0600 Subject: [PATCH 59/62] Fix failed tests --- sanic/app.py | 28 +++++++++---------- tests/test_exceptions_handler.py | 48 +++++++++++++++++--------------- 2 files changed, 40 insertions(+), 36 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 46667ff8ff..85af9b08ed 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -754,20 +754,20 @@ async def handle_exception( ) if handler: error_logger.warning( - "An error occured while handling the request after at ", - "least some part of the response was sent to the client. ", - "Therefore, the response from your custom exception ", - f"handler {handler.__name__} will not be sent to the ", - "client. Beginning in v22.6, Sanic will stop executing ", - "custom exception handlers in this scenario. Exception ", - "handlers should only be used to generate the exception ", - "responses. If you would like to perform any other ", - "action on a raised exception, please consider using a ", - "signal handler like ", - '`@app.signal("http.lifecycle.exception")`\n', - "For further information, please see the docs: ", - "https://sanicframework.org/en/guide/advanced/", - "signals.html", + "An error occurred while handling the request after at " + "least some part of the response was sent to the client. " + "Therefore, the response from your custom exception " + f"handler {handler.__name__} will not be sent to the " + "client. Beginning in v22.6, Sanic will stop executing " + "custom exception handlers in this scenario. Exception " + "handlers should only be used to generate the exception " + "responses. If you would like to perform any other " + "action on a raised exception, please consider using a " + "signal handler like " + '`@app.signal("http.lifecycle.exception")`\n' + "For further information, please see the docs: " + "https://sanicframework.org/en/guide/advanced/" + "signals.html" ) try: response = self.error_handler.response(request, exception) diff --git a/tests/test_exceptions_handler.py b/tests/test_exceptions_handler.py index 6e763c8105..a0de67373a 100644 --- a/tests/test_exceptions_handler.py +++ b/tests/test_exceptions_handler.py @@ -1,6 +1,7 @@ import asyncio import logging +from typing import Callable, List from unittest.mock import Mock import pytest @@ -257,39 +258,42 @@ def test_error_handler_noisy_log( def test_exception_handler_response_was_sent( - app: Sanic, caplog: LogCaptureFixture + app: Sanic, + caplog: LogCaptureFixture, + message_in_records: Callable[[List[logging.LogRecord], str], bool], ): exception_handler_ran = False @app.exception(ServerError) - def exception_handler(request, exception): + async def exception_handler(request, exception): nonlocal exception_handler_ran exception_handler_ran = True - return text("OK") + return text("Error") - @app.route("/") - async def handler(request: Request): + @app.route("/1") + async def handler1(request: Request): response = await request.respond() await response.send("some text") raise ServerError("Exception") + @app.route("/2") + async def handler2(request: Request): + response = await request.respond() + raise ServerError("Exception") + with caplog.at_level(logging.WARNING): - _, response = app.test_client.get("/") + _, response = app.test_client.get("/1") assert "some text" in response.text - depreciated_warning_issued = False - for record in caplog.records: - if record.message.startswith( - "An error occurred while handling the request after at ", - "least some part of the response was sent to the client. ", - "Therefore, the response from your custom exception ", - ): - depreciated_warning_issued = True - break - - assert depreciated_warning_issued - assert exception_handler_ran - - # In the future version: - # assert not exception_handler_ran - # removing asserting for the depreciated-warning + # Change to assert warning not in the records in the future version. + message_in_records( + caplog.records, + ( + "An error occurred while handling the request after at " + "least some part of the response was sent to the client. " + "Therefore, the response from your custom exception " + ), + ) + + _, response = app.test_client.get("/2") + assert "Error" in response.text From fc673bbd44f6ef7c83df5953bf0b91f8623e1e02 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Wed, 8 Dec 2021 23:40:03 -0600 Subject: [PATCH 60/62] Make DeprecationWarning to be consistant with main branch --- sanic/app.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 85af9b08ed..d77b8c9872 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -42,7 +42,7 @@ Union, ) from urllib.parse import urlencode, urlunparse -from warnings import filterwarnings +from warnings import filterwarnings, warn from sanic_routing.exceptions import ( # type: ignore FinalizationError, @@ -753,7 +753,7 @@ async def handle_exception( exception, request.name if request else None ) if handler: - error_logger.warning( + warn( "An error occurred while handling the request after at " "least some part of the response was sent to the client. " "Therefore, the response from your custom exception " @@ -767,7 +767,8 @@ async def handle_exception( '`@app.signal("http.lifecycle.exception")`\n' "For further information, please see the docs: " "https://sanicframework.org/en/guide/advanced/" - "signals.html" + "signals.html", + DeprecationWarning, ) try: response = self.error_handler.response(request, exception) From 430ae12da86c7f10f6f5d75c1f454a3e54993acb Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 9 Dec 2021 02:55:08 -0600 Subject: [PATCH 61/62] Check request.responded logic updated --- sanic/app.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index d77b8c9872..d0fa76fd3b 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -920,17 +920,21 @@ async def handle_request(self, request: Request): # no cov if isawaitable(response): response = await response - if response is not None and not request.responded: - response = await request.respond(response) - elif not hasattr(handler, "is_websocket") or request.responded: - if request.stream is not None: - response = request.stream.response - if request.responded: + if request.responded: + if response is not None: error_logger.error( "The response object returned by the route handler " "will not be sent to client. The request has already " "been responded to." ) + if request.stream is not None: + response = request.stream.response + elif response is not None: + response = await request.respond(response) + elif not hasattr(handler, "is_websocket"): + response = request.stream.response # type: ignore + + # Make sure that response is finished / run StreamingHTTP callback if isinstance(response, BaseHTTPResponse): From 49453c767f55a480a1d258cb6ced6598c2332cb8 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Thu, 9 Dec 2021 03:11:34 -0600 Subject: [PATCH 62/62] Fix lint --- sanic/app.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index d0fa76fd3b..f02301657f 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -934,8 +934,6 @@ async def handle_request(self, request: Request): # no cov elif not hasattr(handler, "is_websocket"): response = request.stream.response # type: ignore - - # Make sure that response is finished / run StreamingHTTP callback if isinstance(response, BaseHTTPResponse): await self.dispatch(