Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent sending multiple or mixed responses on a single request #2327

Merged
merged 72 commits into from Dec 9, 2021
Merged
Show file tree
Hide file tree
Changes from 59 commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
ff6972f
Nicer error handling after eof, waning for second res created
ChihweiLHBird Nov 26, 2021
978412a
Fix type hint
ChihweiLHBird Nov 27, 2021
7b4cd01
Prevent second call to request.respond in handler.
ChihweiLHBird Nov 27, 2021
d6e38af
Move stream state detect logic from http to response
ChihweiLHBird Nov 27, 2021
584c5bc
Update exception handling for prevent second response
ChihweiLHBird Nov 28, 2021
7af89ef
Add test cases
ChihweiLHBird Nov 28, 2021
f3c9dd6
Clean up new test cases
ChihweiLHBird Dec 1, 2021
c79cb8e
handle exception in request.reset_response
ChihweiLHBird Dec 1, 2021
7cc05dd
More clear exception raising logic in Request.respond
ChihweiLHBird Dec 1, 2021
23956ed
Cleanup and make app doesn't send err page when another res was sent
ChihweiLHBird Dec 1, 2021
bc873f1
Fix test case
ChihweiLHBird Dec 1, 2021
25e0eb0
Fix
ChihweiLHBird Dec 1, 2021
df9c404
raise an exception when request.respond is called and another res was…
ChihweiLHBird Dec 1, 2021
08c3eb4
Remove broken tests
ChihweiLHBird Dec 1, 2021
94d8db9
Undo change
ChihweiLHBird Dec 1, 2021
8fe123b
Create new exception type: ResponseException; fixed msgs
ChihweiLHBird Dec 2, 2021
bb83527
Fix msg
ChihweiLHBird Dec 2, 2021
8ee9270
Only run res middleware when res is new
ChihweiLHBird Dec 2, 2021
b65ee67
Update test case
ChihweiLHBird Dec 2, 2021
83b4aba
Fix msg
ChihweiLHBird Dec 2, 2021
777e9f6
Fix msg
ChihweiLHBird Dec 2, 2021
3dab5fc
Fix re_use_res
ChihweiLHBird Dec 2, 2021
be9b4cd
Fix test case
ChihweiLHBird Dec 2, 2021
153be3c
Refactor some logiscs; other minor changes
ChihweiLHBird Dec 2, 2021
893d432
Fix type checking
ChihweiLHBird Dec 2, 2021
8ca425f
Fix lint
ChihweiLHBird Dec 2, 2021
2c40907
Candidate 2 implementation
ChihweiLHBird Dec 2, 2021
21fab64
Merge branch 'main' into zhiwei/dev-candidate2
ChihweiLHBird Dec 2, 2021
62c1dcb
Try adding stage to asgi
ChihweiLHBird Dec 3, 2021
1ce2b44
Disconnect stream when new res generated (code by @Tronic)
ChihweiLHBird Dec 3, 2021
bcae910
Fix typing
ChihweiLHBird Dec 3, 2021
c9eb6f0
Fix
ChihweiLHBird Dec 3, 2021
d43f74b
Disconnect stream from response when resetting
ChihweiLHBird Dec 3, 2021
7a2c684
Remove unnecessary test
ChihweiLHBird Dec 3, 2021
fbd4dfd
Clear raising exception logic
ChihweiLHBird Dec 3, 2021
5b8b8bc
Merge branch 'main' into zhiwei/dev-candidate2
ChihweiLHBird Dec 3, 2021
58b3613
Fix lint
ChihweiLHBird Dec 3, 2021
e4b53a0
Fix typing check
ChihweiLHBird Dec 3, 2021
c3524d2
Merge branch 'zhiwei/dev-candidate2' of https://github.com/ChihweiLHB…
ChihweiLHBird Dec 3, 2021
b4bd23a
Fix msg, replace ResponseException with ServerError; add exc_info=Tru…
ChihweiLHBird Dec 3, 2021
214c1ea
Simplefied error raising logic
ChihweiLHBird Dec 3, 2021
32b6b98
Disconnect prev response in agi.respond
ChihweiLHBird Dec 3, 2021
69e8c11
Add response attr to ASGIApp, fix typing
ChihweiLHBird Dec 3, 2021
97b075d
Strean finally be in Stage.IDLE
ChihweiLHBird Dec 4, 2021
bca4508
Merge branch 'main' into zhiwei/dev-candidate2
ChihweiLHBird Dec 4, 2021
8996167
Only set stage to idle when it is in response
ChihweiLHBird Dec 5, 2021
8e67b11
Match http behavior
ChihweiLHBird Dec 5, 2021
5ecac7e
Merge branch 'main' into zhiwei/dev-candidate2
ChihweiLHBird Dec 6, 2021
dd10699
Merge branch 'main' into zhiwei/dev-candidate2
ahopkins Dec 6, 2021
61d56ee
Add the deprecated warning for exception handler
ChihweiLHBird Dec 7, 2021
74a4377
Log an error if a res returned by a route handler when a pre res was …
ChihweiLHBird Dec 7, 2021
ad7c053
Merge branch 'main' into zhiwei/dev-candidate2
ChihweiLHBird Dec 7, 2021
6f5d378
Add a test case and type hints for tests
ChihweiLHBird Dec 8, 2021
cab3f68
Update error log messages
ChihweiLHBird Dec 8, 2021
71e69ab
Update and add test cases
ChihweiLHBird Dec 8, 2021
8be48a2
Fix app
ChihweiLHBird Dec 8, 2021
19717fc
Fix typing and better if statement logic
ChihweiLHBird Dec 8, 2021
cb3abfe
Add test cases for sending data after eof
ChihweiLHBird Dec 8, 2021
64a12a9
Move send after eof case out and tobe a independent case
ChihweiLHBird Dec 8, 2021
37343b2
Update error msg
ChihweiLHBird Dec 8, 2021
d391893
Update error msg2
ChihweiLHBird Dec 8, 2021
a350547
Update error msg3
ChihweiLHBird Dec 8, 2021
4a78dc0
Merge branch 'zhiwei/dev-candidate2' of https://github.com/ChihweiLHB…
ChihweiLHBird Dec 8, 2021
ef2c0ec
Merge branch 'main' into zhiwei/dev-candidate2
ChihweiLHBird Dec 8, 2021
bdd8210
Update error msg
ChihweiLHBird Dec 8, 2021
7793a86
Merge branch 'zhiwei/dev-candidate2' of https://github.com/ChihweiLHB…
ChihweiLHBird Dec 8, 2021
0ad8f52
Fix lint
ChihweiLHBird Dec 8, 2021
61d8ead
Update error msgs in tests
ChihweiLHBird Dec 8, 2021
c8f2c3e
Fix failed tests
ChihweiLHBird Dec 9, 2021
fc673bb
Make DeprecationWarning to be consistant with main branch
ChihweiLHBird Dec 9, 2021
430ae12
Check request.responded logic updated
ChihweiLHBird Dec 9, 2021
49453c7
Fix lint
ChihweiLHBird Dec 9, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 54 additions & 3 deletions sanic/app.py
Expand Up @@ -66,6 +66,7 @@
URLBuildError,
)
from sanic.handlers import ErrorHandler
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 (
Expand Down Expand Up @@ -733,6 +734,47 @@ async def handle_exception(
context={"request": request, "exception": exception},
)

if (
request.stream is not None
and request.stream.stage is not Stage.HANDLER
):
error_logger.exception(exception, exc_info=True)
logger.error(
"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."
)

# ----------------- 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 "
"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"
ChihweiLHBird marked this conversation as resolved.
Show resolved Hide resolved
)
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.")
ChihweiLHBird marked this conversation as resolved.
Show resolved Hide resolved
error_logger.exception(e)
# ----------------------------------------------

return

# -------------------------------------------- #
# Request Middleware
# -------------------------------------------- #
Expand Down Expand Up @@ -762,6 +804,7 @@ async def handle_exception(
)
if response is not None:
try:
request.reset_response()
response = await request.respond(response)
except BaseException:
# Skip response middleware
Expand Down Expand Up @@ -871,10 +914,18 @@ 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
elif not hasattr(handler, "is_websocket") or request.responded:
ahopkins marked this conversation as resolved.
Show resolved Hide resolved
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. "
ChihweiLHBird marked this conversation as resolved.
Show resolved Hide resolved
)

# Make sure that response is finished / run StreamingHTTP callback
if isinstance(response, BaseHTTPResponse):
Expand Down
17 changes: 16 additions & 1 deletion sanic/asgi.py
Expand Up @@ -7,8 +7,10 @@

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.response import BaseHTTPResponse
from sanic.server import ConnInfo
from sanic.server.websockets.connection import WebSocketConnection

Expand Down Expand Up @@ -83,6 +85,8 @@ class ASGIApp:
transport: MockTransport
lifespan: Lifespan
ws: Optional[WebSocketConnection]
stage: Stage
response: Optional[BaseHTTPResponse]

def __init__(self) -> None:
self.ws = None
Expand All @@ -95,6 +99,8 @@ async def create(
instance.sanic_app = sanic_app
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(
Expand Down Expand Up @@ -149,6 +155,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):
Expand All @@ -163,11 +171,17 @@ 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")
if self.response is not None:
self.response.stream = None
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(
Expand Down Expand Up @@ -195,6 +209,7 @@ 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:
await self.sanic_app.handle_exception(self.request, e)
5 changes: 5 additions & 0 deletions sanic/http.py
Expand Up @@ -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

Expand Down
30 changes: 27 additions & 3 deletions sanic/request.py
Expand Up @@ -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
Expand All @@ -32,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, ServerError
from sanic.headers import (
AcceptContainer,
Options,
Expand All @@ -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
Expand Down Expand Up @@ -104,6 +104,7 @@ class Request:
"parsed_json",
"parsed_forwarded",
"raw_url",
"responded",
"request_middleware_started",
"route",
"stream",
Expand Down Expand Up @@ -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__
Expand All @@ -164,6 +166,21 @@ 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 ServerError(
"Cannot reset response because previous response was sent."
)
self.stream.response.stream = None
self.stream.response = None
ChihweiLHBird marked this conversation as resolved.
Show resolved Hide resolved
self.responded = False
except AttributeError:
pass

async def respond(
self,
response: Optional[BaseHTTPResponse] = None,
Expand All @@ -172,13 +189,19 @@ 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 ServerError("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(
response = HTTPResponse(
status=status,
headers=headers,
content_type=content_type,
)

# Connect the response
if isinstance(response, BaseHTTPResponse) and self.stream:
response = self.stream.respond(response)
Expand All @@ -193,6 +216,7 @@ async def respond(
error_logger.exception(
"Exception occurred in one of response middleware handlers"
)
self.responded = True
return response

async def receive_body(self):
Expand Down
20 changes: 17 additions & 3 deletions sanic/response.py
Expand Up @@ -3,6 +3,7 @@
from os import path
from pathlib import PurePath
from typing import (
TYPE_CHECKING,
Any,
AnyStr,
Callable,
Expand All @@ -19,11 +20,15 @@
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, ServerError
from sanic.helpers import has_message_body, remove_entity_headers
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:
Expand All @@ -45,7 +50,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[Union[Http, ASGIApp]] = None
self.status: int = None
self.headers = Header({})
self._cookies: Optional[CookieJar] = None
Expand Down Expand Up @@ -112,8 +117,17 @@ 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 is None:
raise SanicException(
"Stream was disconnected with this response instance."
)
ChihweiLHBird marked this conversation as resolved.
Show resolved Hide resolved
if self.stream.send is None:
if end_stream and not data:
return
raise ServerError(
"Response stream was ended, no more response data is "
"allowed to be sent."
)
data = (
data.encode() # type: ignore
if hasattr(data, "encode")
Expand Down
16 changes: 15 additions & 1 deletion tests/conftest.py
Expand Up @@ -6,7 +6,8 @@
import sys
import uuid

from typing import Tuple
from logging import LogRecord
from typing import Callable, List, Tuple

import pytest

Expand Down Expand Up @@ -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