WebSocket handlers may return an HTTP 204 response

[Klavionik]

Another one about WebSockets. :)

This PR #446, while fixing important things, introduced an interesting bug I didn't expect to happen.

Apparently, if a route handler has a return type of None (like a typical WebSocket handler does), its result will be replaced with an HTTP 204 response. This is really handy for HTTP routes, but now that we have this line in Application._handle_websocket() it may produce an error:

try:
    return await route.handler(ws)  # This line returns HTTP 204.
except Exception as exc:
    ...

The error goes like ASGI callable should return None, but returned '<Response 204>'.

I see two ways to fix this:

1. The implicit one. Just do this and no one will be hurt.

try:
    await route.handler(ws)
    return
except Exception as exc:
    ...

2. The explicit one. Don't normalize handler's result to 204 if it's a WebSocket handler. Even more, ignore any result from a WebSocket handler and always return None. I guess it could be done in _get_async_wrapper_for_output like that:

def _get_async_wrapper_for_output(
    method: Callable[[Request], Any],
) -> Callable[[Request], Awaitable[Response]]:
    @wraps(method)
    async def handler(request: Request) -> Response | None:
        if isinstance(request, WebSocket):
            await method(request)
            return
        return ensure_response(await method(request))

    return handler

I like the explicit one better. I could create a PR to fix this, but I'm not sure if this solution is indeed right (though it fixes the error message and passes the tests).

[RobertoPrevato]

I see, I also like the first option and I don't think it is an implicit behavior. I wouldn't accept option 2.

Should we maybe differentiate like we did when handling exceptions, if the WebSocket was accepted or not, and support controlling the response code before the handshake?

[Klavionik]

I'm ok with the first solution, I'm just slightly worried about creating a 204 reponse and then ignoring it, all in vain. :)

Should we maybe differentiate like we did when handling exceptions, if the WebSocket was accepted or not, and support controlling the response code before the handshake?

Maybe, but for what purpose? As long as there is no exception raised inside the handler, we don't care... I guess?

[RobertoPrevato]

For example, let's say we have an API that handles topics and comments about topics. We have a WebSocket endpoint where a user can subscribe to get new comments about a specific topic through WS (/chat/{topic_id}), passing a Topic ID in the route. The user of BlackSheep might want to return in the HTTP handshake request, a 404 Not Found response if someone tries to subscribe to a non-existent topic, or Bad Request is the topic ID is not valid. The user might return instances of Response and use the methods in blacksheep.server.responses without raising exceptions. 🤔

The MDN mentions: If any header is not understood or has an incorrect value, the server should send a 400 ("Bad Request") response and immediately close the socket. As usual, it may also give the reason why the handshake failed in the HTTP response body, but the message may never be displayed (browsers do not display it).

Even though the message may never be displayed, since we offer the ability to accept the WebSocket explicitly we should also support controlling what happens before accepting it.

[Klavionik]

As we discussed earlier, there is no way in the ASGI world to send a user-defined HTTP response during the handshake phase.

If we would try to send an HTTP response before accepting a WS connection, using Uvicorn we would see an error: ASGI callable returned without sending handshake. Using Daphne, there's no error message in the log, but a WSDISCONNECT message after 5 seconds.

It seems like raising an exception to make the ASGI server reject the connection with 403 status is the only option we have as application developers.

[RobertoPrevato]

Sorry for my confusion. About the other problem you mentioned, creating a 204 response in vain, the most efficient way is to not normalize the web socket request handler in the same way like the others (so an if check is done only once at application start and not once per every web request). I can look into that.

[RobertoPrevato]

Hi @Klavionik
Thank You again for reporting this issue, I fixed it at 8a26922 - I also improved the normalization logic to not normalize the output of WebSocket handlers, as I mentioned in my previous message.