Clarifying HTTP exception behavior wrt. 500 responses.

[tomchristie]

Related: #54

Having worked through the implementation implications in both uvicorn and starlette, I'm now confident that the behaviors should be:

• Servers should log the exception. The must check if a response has been started, and must send a 500 if no response has been sent. They should not issue any particular warning if an exception is raised once a response has already been sent, as this represents normal behavior for a well-behaved application, although they may issue an explicit warning in the case of an exception where the response has started but not completed.
• Applications may choose to catch exceptions, and send an appropriate 500 page (eg. debug traceback or user-facing 500 error page) if the response has not yet started. If they do so then they must subsequently still raise the exception. This behavior enables the server to continue to log the traceback as desired, and makes the exception visible to any error-monitoring middleware.

Both Uvicorn's WSGI middleware and asgiref's WsgiToAsgi appear to have incorrect behavior here, in that they raise exc_info immediately in both cases, rather than sending an application's 500 response that is returned alongside with the exc_info and then raising the exception. (End result - the server gets to log the exception and returns a 500 response, but we don't get any application-defined 500 responses, either traceback or user-facing 500s)

An example of what I've used to test server/application interaction for resolving this wrt uvicorn's middleware... Taken from the WSGI spec

import uvicorn
import sys


def app(environ, start_response):
    try:
        # regular application code here
        raise RuntimeError()
        status = "200 Froody"
        response_headers = [("content-type", "text/plain")]
        start_response(status, response_headers)
        return [b"normal body goes here"]
    except:
        # XXX should trap runtime issues like MemoryError, KeyboardInterrupt
        #     in a separate handler before this bare 'except:'...
        status = "500 Oops"
        response_headers = [("content-type", "text/plain")]
        start_response(status, response_headers, sys.exc_info())
        return [b"error body goes here"]


if __name__ == '__main__':
    uvicorn.run(app, wsgi=True)

Here's my resolution to the issue in uvicorn - note that exc_info needs to be raised not at the end of start_response (since we don't have the response body yet) but after all of the returned byte-iterator has been sent to the send channel.

[tomchristie]

Behavior now changed in uvicorn 0.3.5.

[andrewgodwin]

So what needs doing here - clarification in the spec and also a fix to WsgiToAsgi?

[tomchristie]

Not some much a clarification in the spec (since it doesn't yet discuss exceptions from the ASGI callable, aside from send/receive exceptions), as an addition.

I suppose at some point the HTTP section ought to have an Exceptions section, which details the differences between WSGI's exc_info, and ASGI's "send the response, then raise the exception".

The main overview currently has an "exceptions" section which could note that "exceptions raised by the ASGI application should close the connection, and may also return an appropriate error message, such as an HTTP 500 response (then link to HTTP spec for fuller details)."

And yes, also a fix to the middleware.

I'll start by considering the docs change as being on my backlog, but equally happy if you or anyone else is able to get there first.

[andrewgodwin]

OK - I won't be able to get to either in the short term, but will leave this open to track the work.

[tomchristie]

Related to this may be some tightening up on specification around how exceptions within send/receive should be treated be applications (i.e. they must be treated as opaque) and making explicit that "send"ing to disconnected connection shouldn't result in an exception, but should be silently allowed. (eg. see discussions about websocket handling in encode/starlette#58 (comment) and encode/starlette#56 which are discussions that arise out of both spec and implementation being a bit wooly there at the moment.)

[andrewgodwin]

Hmm, is there a reason to not raise a known error if you send down a disconnected connection? I know we could just as easily make it vanish into the void, but that seems harder to debug in the edge case.

[tomchristie]

Okay, I'm going to open a seperate ticket to discuss this (#66) since I think it's a bit of a thorny question, and really it's seperate to this ticket which is "how should applications handle exception, particularly wrt. issuing technical 500 responses", and which I think is basically resolved.

[andrewgodwin]

@tomchristie Do you think there's more work we need to do here?