frankie567 / httpx-ws

WebSocket support for HTTPX
https://frankie567.github.io/httpx-ws/
MIT License
110 stars 17 forks source link

Can't send close reason correctly #70

Closed WSH032 closed 3 months ago

WSH032 commented 7 months ago

Describe the bug

When closing a WebSocket connection, httpx-ws can correctly send the close code, but it is unable to send the close reason.

To Reproduce

Run following code

import anyio
import httpx_ws
from fastapi import FastAPI, WebSocket, WebSocketDisconnect
from uvicorn import Config
from uvicorn.server import Server

CODE = 1002
REASON = "Bye"

PORT = 8699

app = FastAPI()

@app.websocket("/")
async def websocket(ws: WebSocket):
    await ws.accept()
    await ws.send_text("Hello, world!")
    try:
        await ws.receive_text()
    except WebSocketDisconnect as e:
        print(e)

        # NOTE: here we find that the close reason is not what we expected
        print("is wrong reason?", e.reason != REASON)

server = Server(Config(app=app, port=PORT))

async def main():
    async with httpx_ws.aconnect_ws(f"ws://127.0.0.1:{PORT}/") as ws:
        print(await ws.receive_text())
        await ws.close(CODE, REASON)

async with anyio.create_task_group() as tg:
    tg.start_soon(server.serve)
    # wait for server to start
    await anyio.sleep(5)
    tg.start_soon(main)
    # wait for main to finish
    await anyio.sleep(5)

    server.should_exit = True

You can see:

(<CloseReason.PROTOCOL_ERROR: 1002>, None)
is wrong reason? True

This is a colab demo, click to run

Expected behavior

WebSocketDisconnect should be (1002, 'Bye')

Configuration

fastapi==0.110.1 httpx-ws==0.6.0 anyio==4.3.0 uvicorn==0.29.0 hypercorn==0.16.0

frankie567 commented 7 months ago

I think we've found a bug in Uvicorn ๐Ÿ™ƒ I've digged this and it turns out that Uvicorn omits to set the reason when they receive a close event, both in the wsproto and websockets implementation:

https://github.com/encode/uvicorn/blob/a2219eb2ed2bbda4143a0fb18c4b0578881b1ae8/uvicorn/protocols/websockets/wsproto_impl.py#L214

https://github.com/encode/uvicorn/blob/a2219eb2ed2bbda4143a0fb18c4b0578881b1ae8/uvicorn/protocols/websockets/websockets_impl.py#L384

I tried with a simple fix and the issue you described was solved. I'll propose a PR there to fix this ๐Ÿ‘

frankie567 commented 7 months ago

Actually, the problem is a bit more complex. Follow here: https://github.com/encode/uvicorn/discussions/2299

WSH032 commented 7 months ago

Oh, sorry for reporting the wrong issue.

I'm not very familiar with the ASGI specification, and I couldn't find any relevant issues in the repos of uvicorn and hypercorn. This led me to mistakenly think it was an issue with httpx-ws, but actually httpx-ws works fine.

Thank you for your quick response๐Ÿ‘! Perhaps we should close this issue as not planned?

frankie567 commented 7 months ago

No worries! TBH, I was surprised to discover that it was actually not my fault ๐Ÿ˜›

I would like to keep this open for the time being to keep track of this and see if we can push lines downstream!

WSH032 commented 7 months ago

Great, thank you! Solving this problem will also benefit my downstream libraries.

frankie567 commented 3 months ago

This is now solved with uvicorn>=0.30.2.

Very glad that we were able to track down this issue, which lead to fix an omission in the ASGI spec itself ๐Ÿ‘