feat(websocket): add support for reason in WebSocket.close() #2056
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2056 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 55 55
Lines 5553 5587 +34
Branches 880 888 +8
=========================================
+ Hits 5553 5587 +34
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Thanks for the PR @wendy5667, and sorry we've been slow to review this. 👿
It looks great at first glance, particularly additions to the test suite. 💯
I'm yet to read this thoroughly... just posting a couple of potential mprovements off the top of my head:
- We need to write a newsfragment for the new feature you've implemented.
- I'm not sure if we can always
send()areasonindiscriminately inside an ASGI WebSocket protocol message. Maybe it's safer to only include areasoniff we've verified we're dealing with ASGI HTTP+WebSocket protocol spec version 2.3+?
I agree, this seems the safest option |
falcon/asgi/app.py
Outdated
| { | ||
| 'type': EventType.WS_CLOSE, | ||
| 'code': WSCloseCode.SERVER_ERROR, | ||
| 'reason': 'Internal Server Error', |
There was a problem hiding this comment.
here we should probably send reason only if the ver is 2.3 or greater.
This here poses a problem since doing a simple ver >= "2.3" does not work assuming future version, since that check would fail if ver ever goes over "2.10".
we probably need to convert ver to a tuple of int, but I don't know if we can assume that it will always be in the form xx.yy and not have values like 2.3.1 or something. @vytas7 do you have suggestions here?
There was a problem hiding this comment.
I added a new member function in asgi/ws.py to check if the asgi version support reason. Is it more reasonable to put this function in util/misc.py so both asgi/ws.py and asgi/app.py can utilize it?
There was a problem hiding this comment.
I think we could keep it in ws.py, but we could make _check_support_reason a function, so that app could call it too?
There was a problem hiding this comment.
Personally, I can accept >= '2.3' as good enough until proven otherwise; the ASGI WebSocket spec is not evolving that fast and I'm not sure if we live to see 2.10 😈 Moreover, they might cut straight to 3.x which would still work with this simple comparison.
Let's just add a # NOTE for posterity if we go this way.
There was a problem hiding this comment.
since that's not too performance critical (happens once per web socket), I think being correct here is worth it since it's not that much more code
There was a problem hiding this comment.
The function check_support_reason() has been added in ws.py. Hopefully, it could deal with any form of ver string, whether it is 2.3 or 2.10.1.
falcon/testing/helpers.py
Outdated
| return { | ||
| 'type': EventType.WS_DISCONNECT, | ||
| 'code': self._close_code, | ||
| 'reason': self._close_reason, |
There was a problem hiding this comment.
I guess if we decide to keep None reason could be omitted in case of None
There was a problem hiding this comment.
Here I think we could omit reason if empty string?
falcon/testing/helpers.py
Outdated
| return { | ||
| 'type': EventType.WS_DISCONNECT, | ||
| 'code': self._close_code, | ||
| 'reason': self._close_reason, |
There was a problem hiding this comment.
Here I think we could omit reason if empty string?
Could you kindly share the style guide for newsfragments? I couldn't find it in the Contributor's Guide. Thank you! |
Summary of Changes
This PR adds
reasonto Websocket.close() as specified in version 2.3 of the HTTP & WebSocket ASGI Message Format. Closure due toHTTPErrors are rendered automatically from HTTP status code, while others are rendered by a default mapping, which could be changed by users.Related Issues
Closes #2025
Pull Request Checklist
docs/.docs/.versionadded,versionchanged, ordeprecateddirectives.docs/_newsfragments/, with the file name format{issue_number}.{fragment_type}.rst. (Runtowncrier --draftto ensure it renders correctly.)