Websockets: implement reading fragmented message [WIP]

[jebej]

Allow reading fragmented message by checking the final property of each message's header. With this I was able to read a broken-up message from a server properly.

Let me know if this is a good direction, of if some refactoring is needed.

[codecov-io]

Codecov Report

Merging #638 (a5c1a32) into master (ad72f4a) will increase coverage by 1.95%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master     #638      +/-   ##
==========================================
+ Coverage   75.59%   77.54%   +1.95%     
==========================================
  Files          36       36              
  Lines        2151     2320     +169     
==========================================
+ Hits         1626     1799     +173     
+ Misses        525      521       -4     

Impacted Files	Coverage Δ
src/WebSockets.jl	85.31% <96.29%> (+4.01%)	⬆️
src/RetryRequest.jl	55.00% <0.00%> (-2.90%)	⬇️
src/Messages.jl	92.48% <0.00%> (-0.86%)	⬇️
src/AWS4AuthRequest.jl	79.36% <0.00%> (-0.30%)	⬇️
src/HTTP.jl	100.00% <0.00%> (ø)
src/ascii.jl	100.00% <0.00%> (ø)
src/layers.jl	100.00% <0.00%> (ø)
src/Strings.jl	100.00% <0.00%> (ø)
src/MessageRequest.jl	100.00% <0.00%> (ø)
src/TimeoutRequest.jl	100.00% <0.00%> (ø)
... and 20 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad72f4a...a5c1a32. Read the comment docs.

[quinnj]

Oof, this seems like hefty changes without a goto person to review; could you add tests around the change in functionality?

[jebej]

I added some tests to read a fragmented message when receiving from both a remote and local server.

We actually already had a method to send fragmented messages so that was pretty simple.

[jebej]

If it helps, I have been playing with WebSockets for a little while in Java/MATLAB: https://github.com/jebej/MatlabWebSocket

I added the fragmented message support recently, which is when I noticed it wasn't supported here.

[jebej]

I'm now noticing that there was some support already when using write(buf::IOBuffer, ws::WebSocket), which used to go through this method in Base:

https://github.com/JuliaLang/julia/blob/048bde05db8ca8fa6fbfc80dacc511d57114bf56/base/io.jl#L713-L719

This bypasses looking for the continuation and final frames entirely, and relies instead on an eof.

There are multiple issues with this

1. Even separate messages that are not meant to be combined will be read combined (as is shown in the test),
2. It relies on the connection being eof to find the end (again, instead of looking for FIN), which is quite limiting,
3. In order to get the eof, the last message sent needs to close the connection, in the test this is done via the closewrite method. This method only sends a message telling the server to close the connection, and nothing is done on our end, meaning that we can still write without errors to the socket, though reading will fail.

I'm not sure what is preferred here. I can do a deeper refactor to fix this if desired.

[jebej]

This bypasses looking for the continuation and final frames entirely, and relies instead on an eof.

This was the case before this PR, since write(buf::IOBuffer, ws::WebSocket) uses readavailable, which I modified. Now, readavailable returns complete messages, looking for FIN.

[quinnj]

I can do a deeper refactor to fix this if desired.
Getting things more correct/accurate is always desired! Thanks for all your help on the fixing here. Should we merge this now and do the refactoring later, or would you include it in this PR?

[jebej]

I changed the PR to keep the same interface for readframe, where it reads only a single frame. It makes more sense name-wise, and prevents breakage. To preserve compatibility I also had to introduce a _readframe method which returns both the payload and header.

readuntil now calls readmessage instead, where readmessage takes care of assembling the frames by calling _readframe.

I also fixed a type-instability: previously readframe would return either an array or a view into an array.

[jebej]

I think the PR is in pretty good shape as-is. A refactor can build on top of this in an other PR.

I actually fixed a bug when receiving a pong in the middle of multiple frames: before readframe would return the pong frame payload, which would have corrupted the data when reading frames via write(buf, ws) (since the pong payload would have been inserted in between).

I added a test for this by manually writing out a fragmented message with a ping.

[quinnj]

Ok great. Happy to merge now then.

The websockets code in HTTP.jl could certainly use a maintainer if you're willing to help out; obviously this is open-source and there's no obligation at all, but the original author of the websocket code here isn't active in the project anymore and so it hasn't received the level of testing/bug-fixes/updating as the rest of the code.