Add support for reason in WebSocket.close()

[vytas7]

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.

[PushanAgrawal]

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

[vytas7]

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]

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

[wendy5667]

Hi, can I also work on this issue?

[vytas7]

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

[samajain]

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]

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 :smiling_imp:

[samajain]

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

[vytas7]

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

How about https://github.com/falconry/falcon/issues/1010?

[samajain]

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

[wendy5667]

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]

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 https://github.com/encode/uvicorn/issues/1181.

@kgriffs @CaselIT any thoughts on this?

[CaselIT]

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]

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]

Also, how will we address the reason in this case? Can we simply write it as "Internal Server Error"? https://github.com/falconry/falcon/blob/47e3e162bf5bd322ec5c2b9a7722e038d635f69f/falcon/asgi/app.py#L972-L980