Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for reason in WebSocket.close() #2025

Open
vytas7 opened this issue Feb 13, 2022 · 15 comments · May be fixed by #2056
Open

Add support for reason in WebSocket.close() #2025

vytas7 opened this issue Feb 13, 2022 · 15 comments · May be fixed by #2056

Comments

@vytas7
Copy link
Member

vytas7 commented Feb 13, 2022

Support for closing with a reason was added in version 2.3 of the HTTP & WebSocket ASGI Message Format (2021-02-02).

Also decide whether we want a way to derive the reason from HTTPErrors like we do with the close code. Alternatives include just using the error's title, description, and adding a new reason parameter to avoid confusion.

@vytas7 vytas7 added good first issue enhancement needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! labels Feb 13, 2022
@vytas7 vytas7 added this to the Version 3.x milestone Feb 13, 2022
@PushanAgrawal
Copy link

Hello I would like to work on this issue. Could you guide me more into it

@vytas7
Copy link
Member Author

vytas7 commented Feb 14, 2022

Hi @PushanAgrawal!
Great that you're interested in this issue.

As to guidance, could you maybe familiarize yourself with how a WebSocket is closed in a Falcon ASGI app atm? Check out that close() method explicitly, as well as how code is inferred from the HTTPError's status code in the case of exception.
Then, you could also try to open a WebSocket connection from the web browser into your sample app, and see how that code and the reason are surfaced to the browser in order to get a better feel of the WS mechanics.

Just setting a reason itself should be fairly straightforward. Check the ASGI spec how that should be returned in the close event.

Edit: Check also out how a CloseEvent looks like from the browser's perspective.

@vytas7
Copy link
Member Author

vytas7 commented Mar 9, 2022

Hi again @PushanAgrawal, just checking if you are still considering to work on this issue?

@wendy5667
Copy link

Hi, can I also work on this issue?

@vytas7
Copy link
Member Author

vytas7 commented Mar 22, 2022

Hi @wendy5667,
Sure, go ahead! (@PushanAgrawal hasn't responded, so it is fair that you take over.)

@vytas7 vytas7 removed the needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! label Mar 22, 2022
@samajain
Copy link

Hi @vytas7,

I am interested in the project and would like to contribute too. Please let me know if I can help if other contributors don't pan out.

Thanks!

@vytas7
Copy link
Member Author

vytas7 commented Mar 30, 2022

Hi @samajain,
That's awesome that you're interested in contributing! The plan is that @wendy5667 is working on this one for now as it's probably not wise to duplicate effort.

Are you interested only in this particular WebSocket issue, or could we find you another one? There is no shortage AFAIC 😈

@samajain
Copy link

samajain commented Apr 3, 2022

I can contribute in on other good-first-issues too! Let me know where I can start.

@vytas7
Copy link
Member Author

vytas7 commented Apr 3, 2022

Heh @samajain it depends what type of issues you like, programming new features, infrastructure, maintenance or documentation.

How about #1010?

@samajain
Copy link

samajain commented Apr 4, 2022

I have experience coding in python, C++ and bash. I can have a look at #1010

@wendy5667
Copy link

wendy5667 commented Apr 15, 2022

Hi @vytas7,

I've done reading the code and found out that there are only limited number of possible Websocket close code. The possibilities that I found are:

  • 3000+http_status_code: when http error happens
  • 1011 or 3011: server internal error
  • 1000: normal
  • client custom code: sent by clients when they disconnect first.

I am thinking we could build a mapping between the Websocket close code and the reason why connection could fail. The mapping could be store in the asgi_spec.py file. Please let me know if this sounds reasonable.

Also, I found that the client would get the code Websocket close 1006 instead of 3xxx when an http error happens. Is this what's supposed to happen? Not sure if I've done something wrong though.

Thank you!

@vytas7
Copy link
Member Author

vytas7 commented Apr 17, 2022

Hi again @wendy5667,
and thanks for the comprehensive investigation/design on this! Much appreciated.

I do not, however, think that we should be performing automatic mapping between the close code and the eventual reason, that feels like too much magic behind the user's back. At least IMHO. An automatic reason can perhaps be acceptable for WS errors automatically rendered from HTTPErrors (i.e., the 3000+HTTP status code case).

As to the client receiving 1006 instead of the designated error code, not sure about this one; Falcon does theoretically render the correct code in isolation (unit tests). Which ASGI server are you testing with, and how does your test program look like?
At least the popular Uvicorn indeed exhibits a couple of edge case issues where the close code is incorrectly set to 1006, AFAICT, the most relevant probably being encode/uvicorn#1181.

@kgriffs @CaselIT any thoughts on this?

@CaselIT
Copy link
Member

CaselIT commented Apr 18, 2022

I do not, however, think that we should be performing automatic mapping between the close code and the eventual reason

this seems something we could have in the ws options. Like default_close_reasons = {1011: 'Internal Server Error'}. An user can set this to an empty dict to disable the automatic setting of the reason

@wendy5667
Copy link

wendy5667 commented Apr 19, 2022

Hi @vytas7 and @CaselIT,

It actually make more sense. What about we do the default mapping thing if the code is among {1000, 1011, 3011}, and generate reason automatically based on HTTPError if the close code is 3000+HTTP status code?

As to the client receiving 1006 case, it does seem like a Uvicorn's problem. I am testing with a very simple client that tries to setup connection through a non-existing route. Falcon does render the right code 3404 as you mentioned, however, the client can't get what it suppose to be. The testing program is as followed:

let webSocket = new WebSocket('ws://localhost:8000/hello/wendy/123');
webSocket.onmessage = function(e) {
    console.log(e)
}
webSocket.onclose = function(e) {
    console.log(e)
}

@wendy5667
Copy link

Also, how will we address the reason in this case? Can we simply write it as "Internal Server Error"?

falcon/falcon/asgi/app.py

Lines 972 to 980 in 47e3e16

async def _handle_websocket(self, ver, scope, receive, send):
first_event = await receive()
if first_event['type'] != EventType.WS_CONNECT:
# NOTE(kgriffs): The handshake was abandoned or this is a message
# we don't support, so bail out. This also fulfills the ASGI
# spec requirement to only process the request after
# receiving and verifying the first event.
await send({'type': EventType.WS_CLOSE, 'code': WSCloseCode.SERVER_ERROR})
return

@wendy5667 wendy5667 linked a pull request Apr 21, 2022 that will close this issue
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants