-
Notifications
You must be signed in to change notification settings - Fork 8
Surface non-Connect handler exceptions to user #219
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -232,7 +232,10 @@ async def __call__( | |
| ctx, | ||
| ) | ||
| except Exception as e: | ||
| return await self._handle_error(e, ctx, send) | ||
| await self._handle_error(e, ctx, send) | ||
| if not isinstance(e, (ConnectError, HTTPException)): | ||
| raise | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When inside an |
||
| return None | ||
|
|
||
| # Streams have their own error handling so move out of the try block. | ||
| return await self._handle_stream( | ||
|
|
@@ -486,6 +489,8 @@ async def _watch_for_disconnect() -> None: | |
| "more_trailers": False, | ||
| } | ||
| ) | ||
| if error and not isinstance(error, ConnectError): | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only raise HTTPExceptions before negotiating a response so don't worry here |
||
| raise error | ||
|
|
||
| async def _handle_error( | ||
| self, exc: Exception, ctx: RequestContext | None, send: ASGISendCallable | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import base64 | ||
| import functools | ||
| import traceback | ||
| from abc import ABC, abstractmethod | ||
| from dataclasses import replace | ||
| from http import HTTPStatus | ||
|
|
@@ -46,9 +47,9 @@ | |
| from .compression import Compression | ||
|
|
||
| if sys.version_info >= (3, 11): | ||
| from wsgiref.types import StartResponse, WSGIEnvironment | ||
| from wsgiref.types import ErrorStream, StartResponse, WSGIEnvironment | ||
| else: | ||
| from _typeshed.wsgi import StartResponse, WSGIEnvironment | ||
| from _typeshed.wsgi import ErrorStream, StartResponse, WSGIEnvironment | ||
| else: | ||
| StartResponse = "wsgiref.types.StartResponse" | ||
| WSGIEnvironment = "wsgiref.types.WSGIEnvironment" | ||
|
|
@@ -251,6 +252,7 @@ def __call__( | |
|
|
||
| except Exception as e: | ||
| _drain_request_body(environ) | ||
| _maybe_log_exception(environ, e) | ||
| return self._handle_error(e, ctx, start_response) | ||
|
|
||
| def _handle_unary( | ||
|
|
@@ -502,6 +504,7 @@ def _handle_stream( | |
| # response message will be handled by _response_stream, so here we have a | ||
| # full error-only response. | ||
| _drain_request_body(environ) | ||
| _maybe_log_exception(environ, e) | ||
| _send_stream_response_headers( | ||
| start_response, protocol, codec, resp_compression.name(), ctx | ||
| ) | ||
|
|
@@ -668,3 +671,12 @@ def _drain_request_body(environ: WSGIEnvironment) -> None: | |
| # server that doesn't do so, so we go ahead and do it ourselves. | ||
| for _ in _read_body(environ): | ||
| pass | ||
|
|
||
|
|
||
| def _maybe_log_exception(environ: WSGIEnvironment, exc: Exception) -> None: | ||
| if isinstance(exc, (ConnectError, HTTPException)): | ||
| return | ||
| errors: ErrorStream = environ["wsgi.errors"] | ||
| errors.write( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: not sure, but should we
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good diligence. I'm leaning towards leaving it out though since I can't imagine a server not writing error messages directly, but also the following |
||
| f"Exception in WSGI application\n{''.join(traceback.format_exception(type(exc), exc, exc.__traceback__))}" | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming we need this for https://github.com/curioswitch/pyqwest/releases/tag/v0.5.1:
Assuming the transparent wheel change in https://github.com/curioswitch/pyqwest/releases/tag/v0.5.0 doesn't need to be called out on our end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so - we never really called out the native aspect clearly before I think, at least in compatibility terms. The perf aspect is probably too much detail on the consumer side (it's pretty small)