Teach pywebsocket to send empty close messages.

[GoogleCodeExporter]

What steps will reproduce the problem?
1.  Try to close a websocket with 'None' as code

What is the expected output? What do you see instead?

RFC 6455 says "When sending a Close frame in response, the endpoint typically 
echos the status code it received." (section 5.5.1)  In the (very) common case 
where JS in the browser calls close() with no code, pywebsocket receives a 
frame with no close code (Firefox has been sending 1000, Chrome sends an empty 
close frame: I'm fixing Firefox to also send an empty frame in 
https://bugzilla.mozilla.org/show_bug.cgi?id=748580 . See also 
https://www.w3.org/Bugs/Public/show_bug.cgi?id=16703 )

But pywebsocket doesn't reply in this case with the same code (i.e. no code); 
instead it sends 1000.  Not a big deal, but it also makes it hard to test this 
case, so I tweaked pywebsocket to allow passing "None" to close_connection().

I figured I'd offer the patch in case you're willing to take it in pywebsocket 
itself--I think it's a useful ability for the server to have, and it would keep 
me from having to patch new pywebsocket versions as they come out.

The patch is against v630.  It also changes some debugging messages to show 
what codes/reasons the server receives/sends during shutdown.

The easiest way to test the patch is to put this in a wsh file:

 def web_socket_passive_closing_handshake(request):
  return request.ws_close_code, request.ws_close_reason

and then test with Chrome using a JS script that calls close() with no 
code/reason.

Original issue reported on code.google.com by jduell.m...@gmail.com on 8 May 2012 at 1:45

Attachments:

• pywebsocket_empty_close

[GoogleCodeExporter]

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

• Changed state: Started

[GoogleCodeExporter]

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:

• pywebsocket_empty_close.v2

[GoogleCodeExporter]

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

[GoogleCodeExporter]

> 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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

> 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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

• Changed state: Fixed

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

Now I released version 0.7.5.
Please check it.

Original comment by toyoshim@chromium.org on 15 May 2012 at 7:42