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

Upgrade the WebSocket libraries #1301

Merged
merged 37 commits into from
Jun 3, 2019
Merged

Upgrade the WebSocket libraries #1301

merged 37 commits into from
Jun 3, 2019

Conversation

scottinet
Copy link
Contributor

@scottinet scottinet commented May 16, 2019

Context

First, some context that will help understand some of the changes made in that PR: the following explains why I went back on using ws instead of porting Kuzzle to µWebSockets.js.

Kuzzle currently depends on the obsolete uws module.
Initially I worked on replacing this module with its newer version (µWebSockets.js): but I discovered, once well into the massive needed refactor (and reading a lot of code because of the poor documentation), that despite its claim, this module does not fully implements the RFC. Most notably, it doesn't allow a server to send PING requests and, as always with this library's author, you have the choice of silently coping with the library flaws, or being insulted if you try to ask about the feature or -god forbids- if you are crazy enough to submit a PR.

So, I decided that continuing to use that author's libraries was a loss of time and even dangerous: I discovered about the unimplemented PING/PONG, but there might be other unimplemented RFC features that we might discover later, and we don't need any more bad surprises.
Sure, that library's performances are insane, but we already spent a lot of time in maintenance because of it.

That said, there were a couple of good ideas I picked up while refactoring the WebSocket layer that I decided to keep. So, you might see what seems "unrelated" changes that I had to do with µWebSockets, and since the code was there, I kept it.

Description

Because uws is obsolete and doesn't work on newer Node.js versions (it seems to work with Node.js 10 now, but not with Node.js 12), it was needed to use another library and, for the reasons explained above, it was decided to go back to the reliable and fully RFC compliant ws.

Changes:

  • Kuzzle now keeps track of activity for each opened sockets, to be able to detect stale or idle sockets when PING/PONG is disabled, which is a sensible choice for high amounts of clients connections (e.g. to terminate idle sockets even if the client is still alive). This can be configured using the new idleTimeout parameter in the kuzzlerc file
  • WebSocket's broadcast method, used to send real-time notifications, has been massively optimized: since broadcasting notifications means sending very similar messages to a potentially very large number of sockets, that method now precomputes a RFC-6455 frame, and access directly on the low-level socket layer to send that frame, instead of asking to ws to compute a new frame for each target client socket. Benchmark results show 60k+ notifications/second sent to 10k registered sockets, on a mere laptop
  • The WebSocket entrypoint now uses Set and Map instead of raw objects to keep track of connections and real-time subscriptions: this change has been made because Set/Map are much more suited (performance-wise) to hold data with constantly happening additions/removals
  • Checks made during startup to verify user configuration are now made using assert, as this makes much more sense since they are supposed to make Kuzzle stop

Other changes

  • the HTTP entrypoint has been refactored: no changes were made to the logic. Initially, a refactor was required because µWebSockets opens its own TCP socket, and manages its own HTTP routing, so I had to adapt that entrypoint accordingly and, along the way, I cleaned it up, fixed the outdated/erroneous JSDoc, and made other changes to make it much more readable and maintanable. I reverted the modifications needed by µWebSockets obviously, but I decided to keep the refactor: I just didn't want to throw my work out the window.

@codecov
Copy link

codecov bot commented May 16, 2019

Codecov Report

Merging #1301 into 1-dev will increase coverage by 0.09%.
The diff coverage is 98.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##           1-dev    #1301      +/-   ##
=========================================
+ Coverage   93.7%   93.79%   +0.09%     
=========================================
  Files         98       98              
  Lines       6930     6967      +37     
=========================================
+ Hits        6494     6535      +41     
+ Misses       436      432       -4
Impacted Files Coverage Δ
...pi/core/entrypoints/embedded/protocols/socketio.js 93.15% <100%> (ø) ⬆️
lib/services/broker/wsBrokerServer.js 99.17% <100%> (ø) ⬆️
...pi/core/entrypoints/embedded/protocols/protocol.js 92.85% <100%> (+10.5%) ⬆️
lib/services/broker/wsBrokerClient.js 95.23% <100%> (ø) ⬆️
...ib/api/core/entrypoints/embedded/protocols/http.js 99.31% <100%> (-0.03%) ⬇️
...ib/api/core/entrypoints/embedded/protocols/mqtt.js 95.23% <100%> (ø) ⬆️
lib/api/core/entrypoints/embedded/index.js 98.31% <100%> (ø) ⬆️
...i/core/entrypoints/embedded/protocols/websocket.js 96.27% <97.29%> (+3.19%) ⬆️

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 c5bcad2...4ec0b96. Read the comment docs.

Copy link
Contributor

@Aschen Aschen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done !
Do we have benchmark before and after this improvement ?

@scottinet
Copy link
Contributor Author

scottinet commented May 16, 2019

@Aschen > About the benchmarks, here are the numbers I got with Node.js 10, between this branch and 1-dev:

  • 500 000 messages sent
  • 10 000 clients

All this on my laptop, which is about to die 😅

scenario 1-dev this branch
(control scenario) raw real-time messages, no subscriptions ~11000 requests/s ~10000 requests/s
100 000 geofencing subscriptions ~82000 notifications/s ~64000 notifications/s

edit: something felt wrong with the first numbers I wrote, and I knew I wouldn't find sleep until I figured out why.
This time I really made sure that conditions between uws and ws were the same, and sadly, even with the optimization I added, ws is simply slower than uws. Pity. 😑

@scottinet
Copy link
Contributor Author

@Aschen > sorry, first benchmarks numbers were wrong, I fixed those. 😑

@kuzzleio kuzzleio deleted a comment from codecov bot May 17, 2019
@scottinet scottinet requested a review from Aschen May 17, 2019 09:14
@scottinet
Copy link
Contributor Author

@Aschen > I re-required your review because, as you suggested, I DRYified the protocols configuration, and it involves some code that you might want to check.

SocketIOMock.prototype.init = sinon.stub().resolves(true);
MqttMock.prototype.init = sinon.stub().resolves(true);
for (const Class of [HttpMock, WebSocketMock, SocketIOMock, MqttMock]) {
Class.prototype.init = sinon.stub().resolves(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@alexandrebouthinon
Copy link
Member

⚠️ Do not update this branch since my PR on nightly and CI optimisation has been merged. We still need to check Node 6 integration tests.

@scottinet scottinet changed the title WebSocket compatibility with newer Node.js versions + enhancements Upgrade the WebSocket libraries May 28, 2019
@Aschen Aschen merged commit cd0b88b into 1-dev Jun 3, 2019
@Aschen Aschen deleted the migrate-to-ws branch June 3, 2019 10:04
@Aschen Aschen mentioned this pull request Jun 14, 2019
Aschen added a commit that referenced this pull request Jun 14, 2019
Release 1.8.0

Bug fixes

    [ #1311 ] Fix promise leaks (scottinet)
    [ #1298 ] Fix disabled protocol initialization (Aschen)
    [ #1297 ] Fix timeouts on plugin action returing the request (benoitvidis)
    [ #1288 ] Fix an error when trying a non-partial bulk update (scottinet)
    [ #1286 ] Allows bulk inserts on aliases (benoitvidis)
    [ #1282 ] Scan keys on redis cluster (benoitvidis)
    [ #1279 ] Users must be authenticated to use auth:logout (scottinet)

New features

    [ #1315 ] Add the new Vault module to handle encrypted application secrets (Aschen)
    [ #1302 ] Add write and mWrite (Aschen)
    [ #1305 ] Add pipes & hooks wildcard event (thomasarbona)

Enhancements

    [ #1318 ] Add a maximum ttl to auth:login (benoitvidis)
    [ #1301 ] Upgrade the WebSocket libraries (scottinet)
    [ #1308 ] Events triggering refactor (scottinet)
    [ #1300 ] Collection specifications methods cloisoned to a collection (thomasarbona)
    [ #1295 ] Improve validation error messages (benoitvidis)
    [ #1292 ] Throw an error when the realtime controller is invoked by plugin developers (benoitvidis)
    [ #1257 ] Add ability to define mapping policy for new fields (Aschen)
    [ #1291 ] Fix --help on subcommands (Yoann-Abbes)
    [ #1289 ] Handle ping/pong packets (scottinet)
    [ #1273 ] Fix incomplete access logs (scottinet)

Others

    [ #1317 ] Add ps dependency to plugin-dev Docker image for pm2 (benoitvidis)
    [ #1312 ] Check that .kuzzlerc.sample is well-formed (scottinet)
    [ #1299 ] Add Kuzzle Nightly & Redis 3 and 4 test (alexandrebouthinon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants