-
-
Notifications
You must be signed in to change notification settings - Fork 921
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
feat(websocket): add support for reason in WebSocket.close() #2056
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
areason
indiscriminately inside an ASGI WebSocket protocol message. Maybe it's safer to only include areason
iff we've verified we're dealing with ASGI HTTP+WebSocket protocol spec version 2.3+?
I agree, this seems the safest option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work, I've marked some minor things
falcon/asgi/app.py
Outdated
{ | ||
'type': EventType.WS_CLOSE, | ||
'code': WSCloseCode.SERVER_ERROR, | ||
'reason': 'Internal Server Error', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we decide to keep None
reason could be omitted in case of None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think we could omit reason if empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update and sorry for the delay.
looks better, I've replied to a few comments
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
reason
to Websocket.close() as specified in version 2.3 of the HTTP & WebSocket ASGI Message Format. Closure due toHTTPError
s 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
, ordeprecated
directives.docs/_newsfragments/
, with the file name format{issue_number}.{fragment_type}.rst
. (Runtowncrier --draft
to ensure it renders correctly.)