Closed GoogleCodeExporter closed 9 years ago
Thank you for your contribution.
I'll apply your change after another member's review.
I guess this is your first patch to pywebsocket.
I'm sorry to bother you. but could you finn in following license agreement?
- for individual contributors
http://code.google.com/legal/individual-cla-v1.0.html
- for corporate contributors
http://code.google.com/legal/corporate-cla-v1.0.html
See also legal section in http://code.google.com/p/pywebsocket/
Original comment by toyoshim@chromium.org
on 8 May 2012 at 11:26
OK, signed agreement.
Also, here's a new version of the patch, with one more fix: don't assume
there's a 'reason' if there's a close code: check length before reading
'reason'.
Original comment by jduell.m...@gmail.com
on 8 May 2012 at 8:52
Attachments:
Hi,
Is this second change needed?
Current code, 'message[2:].decode('utf-8', 'replace')' will set empty unicode
object for close frame with empty reason.
FYI: The change based on your patch is here. It's under review state.
Of course, you can make comments if it misses points you are going to fix.
https://codereview.appspot.com/6202059/
Original comment by toyoshim@chromium.org
on 9 May 2012 at 2:12
> Is this second change needed?
Well, there's a difference between an empty Unicode string and None. The
packet on the wire looks different, and if you're writing a wsh handler that
echoes the 'reason' field, for instance, your reply will be different. See also
https://www.w3.org/Bugs/Public/show_bug.cgi?id=16999
Original comment by jduell.m...@gmail.com
on 9 May 2012 at 3:52
I'm not sure how you managed to get rid of my change to
_filter_and_format_frame_object: IIRC, trying to take len(None) caused an
error. Perhaps you have solved it--I haven't looked carefully enough at your
version to know.
Sorry about my bad python skills!
Original comment by jduell.m...@gmail.com
on 9 May 2012 at 4:07
Close frame contains two-bytes code and reason string in UTF-8. But the reason
is not null-terminated string but the length is calculated from frame size.
There is no difference between no reason and empty string reason in WebSocket
framing. Thus we can use empty unicode string to represent both of them safely.
The reason why I removed your change to _filter_and_format_frame_object is
related to this. I assume reason is never None. At least I expect internal code
never passes None. One possible case is that passive_closing_handshake handler
passes None. I'll care this case in the next update.
Thanks!
Original comment by toyoshim@chromium.org
on 9 May 2012 at 4:58
> There is no difference between no reason and empty string reason
> in WebSocket framing.
This will depend on the resolution to the w3c bug I reference in comment 4.
There is definitely a difference in network framing between sending a packet
with no bytes for reason, vs sending one with bytes that equate to empty
Unicode string. And it is legal to send the empty bytes. (Both Chrome and
Firefox do for empty close()). So pywebsocket must be able to handle such an
incoming message (which I assume it does with your version). The question is
whether pywebsocket or a browser is obligated to report these situations
differently (i.e. setting the 'reason' code in JS or python to an empty string,
or to None/null). I was trying to not convert a missing reason into an empty
string, so that if you have this wsh handler:
def web_socket_passive_closing_handshake(request):
return request.ws_close_code, request.ws_close_reason
and pywebsocket received a close msg with no reason, that
'request.close_reason' would be 'None'. This is so that I could write a wsh
handler that replies with the *exact* same close code/reason (including having
both be missing, or the code set, but the reason be missing) as it received.
That's my interpretation of the spirit of this language in RFC 6455, section #
5.5.1:
"When sending a Close frame in response, the endpoint typically echos the
status code it received."
This doesn't actually mention the close reason, though, so there's nothing that
says one can't actually receive (code=1000, no reason bytes) and reply with
(code=1000, reason=empty Unicode string).
We can probably wait for the W3C bug to be resolved, and that will tell us
whether pywebsocket is obligated to follow my plan, or if we can just treat
missing reason as an empty string, as you're doing.
Original comment by jduell.m...@gmail.com
on 9 May 2012 at 5:13
So at least at the browser client level, there is no difference between missing
reason and empty string reason: both show up as empty string:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=16999
Spec doesn't say for servers, but I don't think it really matters.
Cheers,
Jason
Original comment by jduell.m...@gmail.com
on 9 May 2012 at 9:02
Hum... to my understanding, there is no way to express empty string in UTF-8.
Both of missing reason and empty string reason is represented by following
frame in the case of code 1000.
a) [0x88 0x02 0x03 0xe8]
(I omit masking here for readability, and 0x03e8 is 1000 in decimal)
b) [0x88 0x03 0x03 0xe8 0x00]
This frame which could express empty string is invalid because of the same
reason of following frame c) being invalid.
c) [0x88 0x07 0x03 0xe8 0x45 0x4f 0x46 0x00]
d) [0x88 0x06 0x03 0xe8 0x45 0x4f 0x46]
We never use null terminated string here. d) is only valid expression for
code=1000,reason='EOF'.
It means that ws.close(1000) and ws.close(1000,'') send the same close frame a).
If wire protocol required null terminated UTF-8 string for reason, there could
be difference between missing reason a) and empty reason b). There, both of
them were valid.
I think the point of w3c discussion was which JS expression must be applied for
incoming 0 bytes reason which is the shared expression for missing reason and
empty string reason.
Original comment by toyoshim@chromium.org
on 10 May 2012 at 6:58
http://code.google.com/p/pywebsocket/source/detail?r=631
I land reviewed change at r631.
Jason, I guess you need a release containing this change.
Could you check this revision meets your expectation?
Original comment by toyoshim@chromium.org
on 10 May 2012 at 4:03
The code works fine for all my tests.
One thing I notice which should probably be fixed in a new bug: pywebsocket is
allowing code=1005 to be sent over the network, which is a violation of RFC
6455 section 7.4.1. This happens if I have a dumb "reply with whatever code
arrived from the other end" handler:
def web_socket_passive_closing_handshake(request):
return request.ws_close_code, request.ws_close_reason
If the browser sends an empty close frame, this code sees 1005 as ws_close_code
, passes that back to pywebsocket, and it gets sent on the network:
_stream_hybi.Stream: Received close frame (empty body)
_stream_hybi.Stream: Received client-initiated closing handshake
_stream_hybi.Stream: Sent ack for client-initiated closing handshake (code=1005, reason='')
My version avoided this by setting ws_close_code=None, but that's only one way
of fixing this (and maybe not the right one). In the browser we fail clients
that try to call close() with 1005, so you could throw an error here. Or just
convert 1005 to an empty frame. But whatever you do, 1005 shouldn't go out on
the network.
Thanks for taking my patch!
Original comment by jduell.m...@gmail.com
on 14 May 2012 at 7:03
Thank you for checking.
Right. We should check sending close status code if it's not reserved pseudo
code 1005, 1006, and 1015.
I'm going to fix this and will release new version of pywebsocket after this
change.
https://codereview.appspot.com/6203079/
Thanks,
Original comment by toyoshim@chromium.org
on 15 May 2012 at 3:23
Now I released version 0.7.5.
Please check it.
Original comment by toyoshim@chromium.org
on 15 May 2012 at 7:42
Original issue reported on code.google.com by
jduell.m...@gmail.com
on 8 May 2012 at 1:45Attachments: