Surface non-Connect handler exceptions to user#219
Surface non-Connect handler exceptions to user#219anuraaga wants to merge 4 commits intoconnectrpc:mainfrom
Conversation
fa73a27 to
73b011d
Compare
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
73b011d to
485b5d9
Compare
| "more_trailers": False, | ||
| } | ||
| ) | ||
| if error and not isinstance(error, ConnectError): |
There was a problem hiding this comment.
We only raise HTTPExceptions before negotiating a response so don't worry here
| 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.
This should be raise e , isn't it?
There was a problem hiding this comment.
When inside an except block, raise is actually enough to reraise the active exception. This is exercised in the test_async_unhandled_exception_reraised test
stefanvanburen
left a comment
There was a problem hiding this comment.
looks good, just some nits / questions
| if isinstance(exc, (ConnectError, HTTPException)): | ||
| return | ||
| errors: ErrorStream = environ["wsgi.errors"] | ||
| errors.write( |
There was a problem hiding this comment.
nit: not sure, but should we errors.flush() after this?: https://peps.python.org/pep-3333/#input-and-error-streams. Still understanding WSGI's idioms 😅. I'm assuming we ought to as we're the "portable application"...?
There was a problem hiding this comment.
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 For example, to minimize intermingling of data from multiple processes writing to the same error log. doesn't even make sense to me. Preventing intermingling requires passing a chunk of data in a single write call, flush doesn't seem to matter. So I interpret it as a PEP bug, which happens some times
| class RaisingHaberdasher(Haberdasher): | ||
| def make_similar_hats( | ||
| self, request: Size, ctx: RequestContext | ||
| ) -> AsyncIterator[Hat]: |
There was a problem hiding this comment.
nit: should this be NoReturn typed? (also not async def)
| class RaisingHaberdasher(Haberdasher): | ||
| def make_similar_hats( | ||
| self, request: Size, ctx: RequestContext | ||
| ) -> AsyncIterator[Hat]: |
| "Typing :: Typed", | ||
| ] | ||
| dependencies = ["protobuf>=5.28", "pyqwest>=0.4.1"] | ||
| dependencies = ["protobuf>=5.28", "pyqwest>=0.5.1"] |
There was a problem hiding this comment.
assuming we need this for https://github.com/curioswitch/pyqwest/releases/tag/v0.5.1:
- WSGI testing transport registers and exposes wsgi.errors to allow testing handlers that use it
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.
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)
stefanvanburen
left a comment
There was a problem hiding this comment.
lgtm after force-push w/ DCO
Fixes #217
Ended up folding WSGI in too. Could confirm servers render the exceptions