Conversation
merge from head
|
Should I add a test that checks in case a response started that the handler is still called? |
|
@tomchristie Thoughts? |
|
@aviramha I'm not sure I understand the use case here. Can you clarify why we'd want/need to move the call to the installed error handler separately from the other cases? |
The use case are exception handlers not being called on background tasks since the response already started. This changes the logic so the handlers will be called even if response is returned. It's extremely important for usage of error loggers like sentry etc. |
|
Any update? :) |
Kludex
left a comment
There was a problem hiding this comment.
This makes sense to me.
Thanks for the PR @aviramha ! Also, sorry for taking so long to act here.
I've added a test, but I'm unsure what needs to be added to the documentation, as it seems that the following lines already covers what is here:
Specifically, the part: "within the application".
|
I'll wait to see if someone else wants to comment here. |
|
I think this makes sense, for example if users log within their exception handlers. My main two concerns would be:
|
This is only for HTTP, no interaction. |
|
Docs added. Suggestions welcome about the wording, if any. |
|
Looks great. Thanks @Kludex for seeing this through. |
| response = await run_in_threadpool(self.handler, request, exc) | ||
|
|
||
| # This flow overrides the former response, if there was any. | ||
| if not response_started: |
There was a problem hiding this comment.
I don't really understand the change footprint here.
From the discussion I would have thought that the change would be to move everything outside the if not response started block except await response(scope, receive, send).
(Ie. keep the behaviour the same, except for the fact that we always run the handler)
There was a problem hiding this comment.
You mean like this:
request = Request(scope)
if self.debug:
# In debug mode, return traceback responses.
response = self.debug_response(request, exc)
elif self.handler is None:
# Use our default 500 error handler.
response = self.error_response(request, exc)
else:
# Use an installed 500 error handler.
if asyncio.iscoroutinefunction(self.handler):
response = await self.handler(request, exc)
else:
response = await run_in_threadpool(self.handler, request, exc)
if not response_started:
await response(scope, receive, send)?
There was a problem hiding this comment.
I can do that 👍
It doesn't make a practical difference, as the first two conditionals don't perform any logic besides sending back a response object, but I agree it's cleaner.
There was a problem hiding this comment.
Great, yup. I think it's easier to understand that way round. (And it's a more obvious review too.)
|
@tomchristie Would you like to get the last 👍 here? |
EDIT by @Kludex : Closes #739