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

Nicer error handling after eof, warning for second response being created #2323

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 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
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
13 changes: 13 additions & 0 deletions sanic/app.py
Expand Up @@ -66,6 +66,7 @@
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 (
Expand Down Expand Up @@ -735,6 +736,18 @@ async def handle_exception(
context={"request": request, "exception": exception},
)

if isinstance(request.stream, Http) and request.stream.stage in (
Stage.RESPONSE,
Stage.IDLE,
):
error_logger.exception(exception)
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."
)
return

# -------------------------------------------- #
# Request Middleware
# -------------------------------------------- #
Expand Down
4 changes: 4 additions & 0 deletions sanic/exceptions.py
Expand Up @@ -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
38 changes: 26 additions & 12 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, ResponseException
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 @@ -179,20 +179,34 @@ async def respond(
headers=headers,
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
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this better. Let's just use a better variable name: should_reuse_response

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):
Expand Down
17 changes: 13 additions & 4 deletions sanic/response.py
Expand Up @@ -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 ResponseException, 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


Expand Down Expand Up @@ -101,7 +102,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:
"""
Expand All @@ -112,8 +113,16 @@ 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 ResponseException(
"Response stream was ended, no more response data is "
"allowed to be sent."
)
else:
raise SanicException("Send response function isn't available")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add the Stage to this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. I couldn't think about any other situation that the function is None, but I added a else just in case it falls to this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is the correct approach to have a failsafe, but I was just thinking that if that happens maybe we should include a little bit of information about whatever unexpected state was reached.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What info can be included do you think? I couldn't think about more than that.

data = (
data.encode() # type: ignore
if hasattr(data, "encode")
Expand Down
24 changes: 24 additions & 0 deletions tests/test_middleware.py
Expand Up @@ -297,3 +297,27 @@ async def handler(request):

_, response = app.test_client.get("/")
assert response.json["foo"] == "bar"


def test_middleware_return_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()
return resp1

_, response = app.test_client.get("/")
assert response_middleware_run_count == 1
assert request_middleware_run_count == 1
4 changes: 2 additions & 2 deletions tests/test_requests.py
Expand Up @@ -15,8 +15,8 @@
)

from sanic import Blueprint, Sanic
from sanic.exceptions import ServerError
from sanic.request import DEFAULT_HTTP_CONTENT_TYPE, RequestParameters
from sanic.exceptions import SanicException, ServerError
from sanic.request import DEFAULT_HTTP_CONTENT_TYPE, Request, RequestParameters
from sanic.response import html, json, text


Expand Down