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

WebSocket: enable support for per-message compression #2029

Merged
merged 31 commits into from
Mar 26, 2021
Merged

Conversation

scottinet
Copy link
Contributor

@scottinet scottinet commented Mar 22, 2021

Description

Closes #1734

This PR enables per-message compression for websocket connections.

To do this, this PR replaces the ws module with the uWebSockets.js one: the latter is faster, more stable, and not prone to the catastrophic memory fragmentation issue with Node.js (mitigated in later Node.js 12.x versions).

Since µwebsockets uses C sockets, we cannot replace the WebSocket layer without replacing the HTTP one too: we cannot reuse the Node.js HTTP socket for WebSocket, and we don't want Kuzzle to have 2 different network ports for HTTP and WebSocket.
So this PR also lets µws handle HTTP connections, hence the sizeable refactor.

Enhancements

  • The allow-encoding HTTP header is now properly interpreted: before this PR, the priority argument <algorithm>;q=<priority> was ignored
  • Content parsing for multipart form requests is now done by µws, letting us remove the busboy dependency
  • Due to how µws handles idle client connections and heartbeat, I had to rework the websocket options in our RC file:
    • the heartbeat option is now deprecated and ignored
    • the idleTimeout now cannot be deactivated, and its value cannot be set lower than 1000ms. To prevent breaking changes with existing installs, this option is defaulted to 60000ms if set to a value lower than 1000.
    • a new compression option has been added. It takes a boolean (false by default): setting this option to true enables per-message compression
  • Add a configurable rate limiter for websocket connections (disabled by default)

Other changes

  • Debug domain for the network layer has been renamed kuzzle:network:* (from kuzzle:entry-point:*)
  • The server:getConfig removed the entire http configuration from its output, instead of just the HTTP routes one (http.routes). This has been fixed, as not returning the http config is a bit confusing (I know I was perplexed when trying to figure why I didn't have HTTP config in that route output)
  • Default timeout of cucumber.js has been increased from 5000ms to 30000ms, to prevent functional tests to fail in github actions

Bug fixes

  • Numbers as raw request results are now correctly handled (trying to send a number as a raw response results in an error without this PR)
  • Make a realtime functional test (using the functional tests plugin) reentrant, allowing it to be run multiple times on the same node

@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #2029 (2415016) into 2-dev (e8d582a) will decrease coverage by 0.19%.
The diff coverage is 93.79%.

❗ Current head 2415016 differs from pull request most recent head 1e43ad3. Consider uploading reports for the commit 1e43ad3 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev    #2029      +/-   ##
==========================================
- Coverage   93.77%   93.58%   -0.20%     
==========================================
  Files         123      122       -1     
  Lines        7586     7589       +3     
==========================================
- Hits         7114     7102      -12     
- Misses        472      487      +15     
Impacted Files Coverage Δ
lib/api/controllers/serverController.js 92.40% <0.00%> (ø)
lib/api/funnel.js 96.24% <ø> (ø)
lib/config/default.config.js 100.00% <ø> (ø)
lib/core/network/protocols/protocol.js 86.36% <ø> (-4.27%) ⬇️
lib/kuzzle/kuzzle.js 75.52% <0.00%> (-1.08%) ⬇️
lib/config/index.js 95.31% <90.00%> (-2.42%) ⬇️
lib/core/network/protocols/httpMessage.js 94.44% <94.44%> (ø)
lib/core/network/protocols/httpwsProtocol.js 95.14% <95.14%> (ø)
lib/core/network/context.js 100.00% <100.00%> (ø)
lib/core/network/entryPoint.js 97.89% <100.00%> (-0.08%) ⬇️
... and 6 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 e8d582a...1e43ad3. 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.

Welcome back uWS 🎉

lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@scottinet scottinet requested a review from Aschen March 22, 2021 13:10
@scottinet
Copy link
Contributor Author

@Aschen > I applied your requested changes.

features/PluginContext.feature Outdated Show resolved Hide resolved
@Leodau Leodau linked an issue Mar 25, 2021 that may be closed by this pull request
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
lib/core/network/protocols/http+websocket.js Outdated Show resolved Hide resolved
@scottinet scottinet requested a review from Leodau March 26, 2021 08:48
@scottinet
Copy link
Contributor Author

@Leodau > changes applied.

@sonarcloud
Copy link

sonarcloud bot commented Mar 26, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@scottinet scottinet merged commit 306fb46 into 2-dev Mar 26, 2021
@scottinet scottinet deleted the switch-to-uws branch March 26, 2021 09:12
@MathieuVeber MathieuVeber mentioned this pull request Apr 9, 2021
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.

Allow per message compression for websocket
3 participants