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

[RFC] WebSockets proposal #394

Closed
EricForgy opened this issue Mar 7, 2019 · 8 comments
Closed

[RFC] WebSockets proposal #394

EricForgy opened this issue Mar 7, 2019 · 8 comments

Comments

@EricForgy
Copy link
Contributor

EricForgy commented Mar 7, 2019

Hi guys 👋 (In particular, @hustf, @pfitzseb, @quinnj and @samoconnor)

Background

It's been a while since I did any work with websockets, but I am getting back to it now.

The last time around, I was hoping we could do some shootouts between WebSockets.jl and HTTP.WebSockets.jl, pick the best pieces of each and move everything into a single super package, presumably WebSockets.jl.

That hasn't happened yet, but one thing that HAS happened is that WebSockets.jl now depends only on HTTP.jl (no longer HttpServer.jl) so that is good 👍

However, this means that HTTP.jl is now supporting two, nearly identical, packages / modules, i.e. WebSockets.jl and HTTP.WebSockets.jl.

Furthermore, any package that depends on WebSockets.jl will get an indirect dependency on HTTP.jl, which also contains HTTP.WebSockets.jl.

The problem:

Currently, any package that depends on HTTP v0.8.0 can not also depend on WebSockets.jl.

Examples of packages that cannot be used with HTTP v0.8.0 include WebIO.jl, Mux.jl, Blink.jl, PlotlyJS.jl.

The reason: WebSockets.jl has capped the HTTP.jl dependency to v0.7.99 and those packages depend on WebSockets.jl.

Proposal

I see websocket issues being filed here so people are expending effort to make sure that HTTP.WebSockets works, but as a result, there are fewer eyes looking at WebSockets.jl (which is still stuck on HTTP v0.7).

My proposal is to seek proposals on the best way forward 🤓

Let me start...

I think we should just have one websockets package and I think that websockets package, whatever form it takes - old, new, or mixed - should be located at WebSockets.jl, which means retiring HTTP.WebSockets.jl.

If HTTP.WebSockets.jl is retired, then all eyes will be on WebSockets.jl. Any issues will be filed in one location and resolved in one location.

The main issue is the bit twiddling. WebSockets.jl has its bit twiddling and HTTP.WebSockets.jl has its bit twiddling. I'm sure both are fine and - at the end of the day - I don't think it matters which one we adopt.

Here is what I'm thinking:

  • Create a branch on WebSockets.jl, which is basically just HTTP.WebSockets.jl.
  • Combine the tests of WebSockets.jl and HTTP.WebSockets.jl so we don't miss any test from either one.
  • Create a branch to update the current WebSockets.jl to HTTP v0.8.0.
  • Perform a shootout, i.e. benchmark competition including running all tests.
  • Take the best pieces from each one and merge / tag a new super version of WebSockets.jl.
  • Retire HTTP.WebSockets.jl.

If there is only one websockets package, it will be much easier to make sure it stays in lock step with HTTP.jl.

What do you think? Any other proposals?

PS: I've starting working on getting WebSockets.jl to work with HTTP.jl v0.8.0 here: JuliaWeb/WebSockets.jl#134

@EricForgy
Copy link
Contributor Author

That PR ☝️ completes step 1 of my proposal above.

@essenciary
Copy link

essenciary commented Mar 7, 2019

It took me like 30 minutes to swap WebSockets.jl to HTTP.WebSockets.jl in Genie so migrating any existing codebase is not a problem.

What is the reason for having a separate WebSockets package? The current approach seems to be to add more features into HTTP (like Requests). I don't like it and I would very much prefer that I could pick and mix the components that I need. But I think we should be consistent. If we go towards unbundling, we should unbundle all the way.

Another problem is that the current WebSockets.jl package is not developed at the same rate as HTTP (hence the pain in the kazoo while restricting the HTTP version). If we end up in the same place in 6 months, I'd rather see WebSockets properly maintained as part of HTTP.

PS - I'm really not a fan of seeing a lot of features bundled. All the request logic and router logic and throttling and other unnecessary bits slow down the web server itself. I would like to see a fast server. We can't say Julia is fast if people try out the web stack and it comes out slower than ruby.

@EricForgy
Copy link
Contributor Author

I'm nearly done with step 3 above ☝️ , but running out of steam.

Here is the new PR JuliaWeb/WebSockets.jl#137

@EricForgy
Copy link
Contributor Author

We can't say Julia is fast if people try out the web stack and it comes out slower than ruby.

Amen to that 🙌

@essenciary, as you can imagine there is some history to this story and I'm glad you're chiming in to help us map out a good future for Julia's webstack.

According to Github, WebSockets.jl dates back to March 2013 (here is the initial commit). Since then, it has seen 179 commits with 29 contributors.

HTTP.WebSockets.jl was created in January 2018 entirely by Sam independently of WebSockets.jl. Sam spelled out some of his thoughts for doing so here. That is around the time I started getting involved as well. Looking at it again, this issue provides a great history:

JuliaWeb/WebSockets.jl#84.

😊

As I mentioned above, unless someone suggests something better to do, my plan is to:

  1. Get a standalone branch of HTTP.WebSockets.jl up and running on WebSockets.jl [ Done! ]
  2. Get a standalone brand of WebSockets.jl that works with HTTP.jl v0.8.0 [ Almost done! ]
  3. Try to combine all tests from HTTP.WebSockets.jl and WebSockets.jl and get them both to run all tests.
  4. Benchmark
  5. Pick and choose the best parts from both and combine to a new branch.
  6. Benchmark

After a few iterations, I hope we end up with a single websockets package everyone is happy with and can rally behind. At that point, it might make sense to consider retiring one of them. Either retire HTTP.WebSockets.jl and move everyone to WebSockets.jl or vice versa.

Like all of us, time is not a luxury, but I think this is important enough that I dedicated pretty much the entire last two days to trying to work this out 😅

Another option that would be A LOT easier for me, would be to just submit PRs to Mux.jl, WebIO.jl, PlotlyJS.jl etc that move them to HTTP.WebSockets.jl since they already have that dependency, but that would leave WebSockets.jl as an even more isolated package, which I feel would be unfortunate, so I'm trying to find a compromise 🙏

I really value all of your thoughts 🙏

@EricForgy
Copy link
Contributor Author

Yay! Step three is done (basically) 🎉

WebSockets.jl had some logic to check rate limits for throttling, but now this has all been relegated to HTTP.jl so I am not so motivated to fix the throttling tests in WebSockets.jl since it is just testing HTTP.jl.

JuliaWeb/WebSockets.jl#137

I admit, there is something satisfying to see 1,038 tests run AND PASS 😃

@hustf
Copy link

hustf commented Mar 10, 2019

Eric, thanks for putting in the work with the above.

As stated elsewhere, I believe having several implementations (let's not forget Dandelion WebSockets) is an advantage. Extensive show methods, tests, documentation, benchmarks, documentation, deprecations. All important, but you don't want to pull all of that along while developing rapidly. Which I hope we'll be able to continue to do. This is open source, so when something good emerges, we'll simply copy it elsewhere. Not a big cost.

@SimonDanisch
Copy link
Contributor

I just recently updated my code to use HTTP.Websockets - just to go back today.
I got weird EOF read errors, corrupted data and it also seems like the error code for a Websocket error is garbage data, which sometimes is to big to fit into UInt16, so it also errors from time to time on truncate.
So from my experience, I'd recommend removing HTTP.Websockets, even though I really liked the idea of using just one well maintained package.

@quinnj
Copy link
Member

quinnj commented Jun 11, 2022

I just merged an overhaul of the HTTP.WebSockets code that modernizes the API and includes industry-standard testing compliance against the autobahn testsuite.

@quinnj quinnj closed this as completed Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants