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
Comments
Hello I would like to work on this issue. Could you guide me more into it |
Hi @PushanAgrawal! As to guidance, could you maybe familiarize yourself with how a WebSocket is Just setting a Edit: Check also out how a |
Hi again @PushanAgrawal, just checking if you are still considering to work on this issue? |
Hi, can I also work on this issue? |
Hi @wendy5667, |
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! |
Hi @samajain, Are you interested only in this particular WebSocket issue, or could we find you another one? There is no shortage AFAIC 😈 |
I can contribute in on other good-first-issues too! Let me know where I can start. |
I have experience coding in python, C++ and bash. I can have a look at #1010 |
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:
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! |
Hi again @wendy5667, 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 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? |
this seems something we could have in the ws options. Like |
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)
} |
Also, how will we address the reason in this case? Can we simply write it as "Internal Server Error"? Lines 972 to 980 in 47e3e16
|
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
HTTPError
s like we do with the closecode
. Alternatives include just using the error'stitle
,description
, and adding a new reason parameter to avoid confusion.The text was updated successfully, but these errors were encountered: