websockets icon indicating copy to clipboard operation
websockets copied to clipboard

Incorrect use of process_request silently aborts connections

Open Rosuav opened this issue 2 years ago • 3 comments

async def process_request(path, headers):
    return (200, [], b"Hello, world")

async def main():
    async with websockets.serve(connection, "localhost", 8000, process_request=process_request):
        await asyncio.Future()

The documentation for process_request correctly says that the first value should be an HTTPStatus; however, this example passes a plain int instead.

Expected result: An exception is thrown or displayed on the console. (Or ideally, this particular case should work correctly, since IntEnum and integer are mostly interchangeable.)

Actual result: The exception is swallowed by # Last-ditch attempt to avoid leaking connections on errors and the connection is summarily closed with no message to client or console.

Requesting that the last-ditch handler report errors to the console, possibly with a message indicating how this could be suppressed.

Rosuav avatar Oct 06 '23 23:10 Rosuav

I completed your example as follows:

#!/usr/bin/env python

import asyncio
import websockets

async def connection(ws):
    await asyncio.sleep(1)
    await ws.send("it works")

async def process_request(path, headers):
    return (200, [], b"Hello, world")

async def main():
    async with websockets.serve(connection, "localhost", 8000, process_request=process_request):
        await asyncio.Future()

if __name__ == "__main__":
    asyncio.run(main())

It doesn't crash:

$ curl -v http://localhost:8000/
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET / HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/8.1.2
> Accept: */*
>
< HTTP/1.1 200 OK
< Date: Sat, 21 Oct 2023 13:30:35 GMT
< Server: Python/3.11 websockets/12.0.dev36+ged6cb1b
< Content-Length: 12
< Content-Type: text/plain
< Connection: close
<
* Closing connection 0
Hello, world%

aaugustin avatar Oct 21 '23 13:10 aaugustin

It was fixed in #1309..

aaugustin avatar Oct 21 '23 13:10 aaugustin

Ahh cool, that solves this particular issue. It may still be possible to cause other types of errors that will silently abort connections though. Is there any other way to write the handler that will result in the same last-ditch exception handler being triggered?

Rosuav avatar Oct 21 '23 18:10 Rosuav

Based on code inspection, in the old asyncio implementation, you should get an HTTP 500 error and a message at the ERROR log level in that case.

In the new asyncio implementation and the threading implementation, there's a test to check that an HTTP 500 error is returned if process_request or process_response raise an unexpected exception. See test_process_request_raises_exception and test_process_response_raises_exception.

Requesting that the last-ditch handler report errors to the console, possibly with a message indicating how this could be suppressed.

I don't want to do this because it can be very noisy with connection error, misbehaving clients, and internet background noise, depending on the context in which you're deploying websockets.

Configuring logging in Python is tricky: you can configure it only once. (Technically, you can attempt to reconfigure it, but the behavior is so tricky that everyone gets it wrong; that's why I say that you can configure it only once.) Since websockets is a library and doesn't know in which context it will be used, it would be inappropriate from websockets to take over configuration of logging. I expect the developer of the application to do it in a way that makes sense for their context.

See https://websockets.readthedocs.io/en/stable/topics/logging.html#configure-logging

aaugustin avatar Aug 08 '24 06:08 aaugustin