Skip to content
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

Overhaul websockets code #843

Merged
merged 14 commits into from Jun 11, 2022
Merged

Overhaul websockets code #843

merged 14 commits into from Jun 11, 2022

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jun 10, 2022

Big update to websockets code. The work here includes:

  • Reading through RFC 6455 and trying to make our implementation more compliant
  • Getting all tests passing from the https://github.com/crossbario/autobahn-testsuite, which is pretty much the industry standard for testing websocket spec compliance; running the tests currently requires docker, which is only available on ubuntu for github actions, so this PR is my attempt to at least run those tests on linux; they currently pass on my macOS machine running the docker images locally
  • An updated overall API for our websockets code, in particular:
    • WebSocket doesn't subtype IO anymore; it's kind of an IO, but also not really in a lot of ways, and it actually makes it a little awkward because of all the ways it isn't an IO, while not really getting any advantage from the subtyping
    • The API for using a WebSocket includes send(ws, msg) for sending messages, receive(ws) for receiving non-control messages. pings/pongs are handled automatically. You can also iterate the WebSocket object, where each iteration will yield the body of a non-control message, with iteration finishing when the websocket connection is closed. You can send TEXT messages by calling send(ws, msg::AbstractString), BINARY messages by calling send(ws, msg::AbstractVector{UInt8}), and fragmented messages of either type by calling send(ws, iterable) where the iterable iterates elements of type AbstractString or AbstractVector{UInt8} (must all be one or the other, no intermixing). To close the websocket, you call close(ws). To manually send ping/pong, you call ping(ws), pong(ws).

In terms of making/serving websocket connections, those APIs remain the same via WebSockets.open/WebSockets.listen.

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #843 (41cf3fc) into master (8c21c10) will decrease coverage by 2.70%.
The diff coverage is 56.15%.

@@            Coverage Diff             @@
##           master     #843      +/-   ##
==========================================
- Coverage   80.41%   77.71%   -2.71%     
==========================================
  Files          35       35              
  Lines        2635     2755     +120     
==========================================
+ Hits         2119     2141      +22     
- Misses        516      614      +98     
Impacted Files Coverage Δ
src/HTTP.jl 83.05% <ø> (ø)
src/WebSockets.jl 55.55% <54.21%> (-35.42%) ⬇️
src/Servers.jl 88.16% <100.00%> (+3.40%) ⬆️
src/StreamRequest.jl 100.00% <0.00%> (+1.72%) ⬆️
src/access_log.jl 93.33% <0.00%> (+3.33%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@quinnj
Copy link
Member Author

quinnj commented Jun 10, 2022

Sigh......it's proving difficult to get the CI to run the autobahn testsuite here.

@quinnj
Copy link
Member Author

quinnj commented Jun 10, 2022

Ok, this is looking better; we're able to run the autobahn test suite for 64-bit linux & macOS. 32-bit python seems not possible. and windows doesn't seem possible because it requires python 2.7 and the windows "compiler tools" isn't supported for 2.7 any more.

Going to add docs here and then I think it's ready to merge.

@rikhuijzer
Copy link
Contributor

Getting all tests passing from the https://github.com/crossbario/autobahn-testsuite, which is pretty much the industry standard for testing websocket spec compliance

Did this PR already find bugs? 🚀

@quinnj
Copy link
Member Author

quinnj commented Jun 10, 2022

Oh yes, there were PLENTY of bugs in the old implementation.

@quinnj
Copy link
Member Author

quinnj commented Jun 11, 2022

Alright, lots of inline docs added. I think this is ready to go. Again, merging semi-quickly because I have other work coming in, but I would love anyone to look/review things and we can address things in follow up PRs. I plan to review the ~30 open websockets-related issues, which I believe can mostly be closed from the updates here (but we still need to review each to make sure!).

@quinnj quinnj merged commit d619890 into master Jun 11, 2022
@quinnj quinnj deleted the jq/websockets branch June 11, 2022 06:19
@BoZenKhaa
Copy link
Contributor

I love this ♥. Looking at the inline docs and tests, the blocking eof! is no longer necessary? If so, the readme example is up for a facelift as well: https://github.com/JuliaWeb/HTTP.jl#websocket-examples

Can't wait for this to be released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants