Make error handler run always

[aviramha]

EDIT by @Kludex : Closes #739

[aviramha]

Should I add a test that checks in case a response started that the handler is still called?

[aviramha]

@tomchristie Thoughts?

[florimondmanca]

@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?

[aviramha]

@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.

[aviramha]

Any update? :)

[Kludex]

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".

[Kludex]

I'll wait to see if someone else wants to comment here.

[adriangb]

I think this makes sense, for example if users log within their exception handlers.

My main two concerns would be:

• How this interacts with the work to get exception handlers for websockets
• We'll have to document the behavior. I could see users getting confused as to why the response they return from error handlers isn't used

[Kludex]

How this interacts with the work to get exception handlers for websockets

This is only for HTTP, no interaction.

[Kludex]

Docs added. Suggestions welcome about the wording, if any.

[aviramha]

Looks great. Thanks @Kludex for seeing this through.

[tomchristie]

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)

[Kludex]

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)

?

[Kludex]

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.

[tomchristie]

Great, yup. I think it's easier to understand that way round. (And it's a more obvious review too.)

[Kludex]

@tomchristie Would you like to get the last 👍 here?

[tomchristie]

Great, yup.