[WebSocket] Handle ping/pong packets #1289

scottinet · 2019-04-18T09:43:34Z

Description

PING/PONG packets are described in the WebSocket specifications, and are used to:

efficiently detect and clean up dead sockets (a socket can stay opened indefinitly if the client couldn't send a socket closed packet)
keep idle sockets alive

This PR:

adds a new heartbeat configuration under the protocols.websocket part of the .kuzzlerc file
adds a heartbeat to the WebSocket protocol, sending PING packets to client sockets, and mark them as "dead" until a PONG response is received
destroys and cleans up sockets that stayed dead after the heartbeat delay

Other change

Deduplicated the test made on the enabled part of a protocol configuration: the test was made in each protocol implementation, AND in the global entrypoint object during protocols initialization.

How to test

An easy way to test this is to use wscat:

(if not already done) install it with npm -g wscat
connect it to your local kuzzle stack: wscat --connect http://localhost:7512
suspend it with a SIGSTOP signal (e.g. Ctrl+Z on the terminal)

Without this PR: you can go have a lunch, return to your terminal and wake wscat up (with fg), and you can use it again as if nothing happend, even if the socket should have been marked as dead by Kuzzle since it can never respond to any request.

With this PR: if you wait at least 2 minutes by default, and wake wscat up with fg, it will exit immediately with the following message: disconnected
(you need to wait for 2 heartbeats before the socket is disconnected: the 1st one to mark the socket temporarily as "dead", and the second one to clean up the dead socket)

$ wscat --connect http://localhost:7512
connected (press CTRL+C to quit)
> 
[1]  + 24777 suspended  wscat --connect http://localhost:7512
$ fg
[1]  + 24777 continued  wscat --connect http://localhost:7512
disconnected
$

You can follow what happens with this test using debug messages (set the DEBUG environment variable to kuzzle:entry-point:protocols:websocket before starting Kuzzle):

  kuzzle:entry-point:protocols:websocket [090cacd2-8ca5-49b1-bf8a-8b9b37b5882d] creating Websocket connection +4s
  kuzzle:entry-point:protocols:websocket [WebSocket] Heartbeat +56s
  kuzzle:entry-point:protocols:websocket [WebSocket] Heartbeat +60s
  kuzzle:entry-point:protocols:websocket [090cacd2-8ca5-49b1-bf8a-8b9b37b5882d] received a `close` event +2ms
  kuzzle:entry-point:protocols:websocket [090cacd2-8ca5-49b1-bf8a-8b9b37b5882d] onClientDisconnection +1ms

These logs show that the 2nd heartbeat after the wscat connection correctly force closes the socket, since it couldn't respond to the PING packet in time.

scottinet · 2019-04-18T09:52:02Z

lib/api/core/entrypoints/embedded/index.js

-    if (this.config.protocols.socketio.enabled) {
-      this.protocols.socketio = new SocketIoProtocol();
-      this.protocols.socketio.init(this);
+    this.protocols = {


Unrelated change/Boyscout: the test on the enabled boolean property made in the previous version is duplicated in all protocols implementations, so I decided that protocols were responsible of their own configuration and deduplicated the test by removing the ones made by the global entrypoint object.

codecov-io · 2019-04-18T11:26:21Z

Codecov Report

Merging #1289 into 1-dev will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            1-dev    #1289      +/-   ##
==========================================
+ Coverage   93.78%   93.81%   +0.03%     
==========================================
  Files          98       98              
  Lines        6804     6812       +8     
==========================================
+ Hits         6381     6391      +10     
+ Misses        423      421       -2

Impacted Files	Coverage Δ
...pi/core/entrypoints/embedded/protocols/socketio.js	`93.05% <100%> (ø)`	⬆️
...i/core/entrypoints/embedded/protocols/websocket.js	`93.07% <100%> (+1.04%)`	⬆️
...ib/api/core/entrypoints/embedded/protocols/mqtt.js	`94.11% <100%> (ø)`	⬆️
lib/api/core/entrypoints/embedded/index.js	`98.83% <100%> (+1.04%)`	⬆️

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 a95a819...486aa88. Read the comment docs.

ballinette · 2019-04-18T13:12:22Z

.kuzzlerc.sample

@@ -376,8 +376,14 @@
        // value of Access-Control-Allow-Origin header to answer the upgrade request
        "origins": "*:*"
      },
+      // * enabled:


nitpicking: to stay coherent with other settings above in the file, the comments should be placed "inside" the object:

"websocket": { //  enabled: true, heartbeat: 60000 }

benoitvidis

I suppose that as the PING/PONG implementation is part of the Websocket RFC, the answer to the following question is yes, but are we sure the client implementation of Websockets used by the SDK will reply to the ping requests issued by Kuzzle?

ballinette · 2019-04-18T13:16:34Z

lib/api/core/entrypoints/embedded/protocols/websocket.js

@@ -52,12 +53,27 @@ class WebsocketProtocol extends Protocol {
   */
  init (entryPoint) {
    super.init(entryPoint);


Nitpicking: no need to init the superclass if the protocol is disabled. So we could move this line after the next conditional statement ?

Good point. I made the change for other protocols as well, as this can be confusing to have a debug message saying that we're initializing an actually disabled protocol.

ballinette · 2019-04-18T13:28:58Z

@benoitvidis

I suppose that as the PING/PONG implementation is part of the Websocket RFC, the answer to the following question is yes, but are we sure the client implementation of Websockets used by the SDK will reply to the ping requests issued by Kuzzle?

The SDK use either the native WebSocket object when used inside a browser, or the "ws" package when used in NodeJS, which implements the ping/pong mechanism according to the specs:
https://www.npmjs.com/package/ws#how-to-detect-and-close-broken-connections
Pong messages are automatically sent in response to ping messages as required by the spec.

scottinet · 2019-04-18T13:44:55Z

@benoitvidis > Yes: any websocket library worth its salt will answer to a PING request with a PONG one, as this is part of the RFC.
In fact, I checked it, and this is true for Kuzzle itself: sending it a PING request will make it respond with a PONG one.

scottinet · 2019-04-18T13:49:34Z

@ballinette > I found your remarks to be more than nitpickings. I made the necessary changes 👍

# [1.7.3](https://github.com/kuzzleio/kuzzle/releases/tag/1.7.3) (2019-04-29) #### Bug fixes - [ [#1288](#1288) ] [bulk] fix an error when trying a non-partial bulk update ([scottinet](https://github.com/scottinet)) - [ [#1286](#1286) ] [bugfix] allows bulk inserts on aliases ([benoitvidis](https://github.com/benoitvidis)) - [ [#1282](#1282) ] [bugfix] scan keys on redis cluster ([benoitvidis](https://github.com/benoitvidis)) - [ [#1279](#1279) ] Users must be authenticated to use auth:logout ([scottinet](https://github.com/scottinet)) #### Enhancements - [ [#1292](#1292) ] KZL 1032 - Throw an error when the realtime controller is invoked by plugin developers ([benoitvidis](https://github.com/benoitvidis)) - [ [#1257](#1257) ] Add ability to define mapping policy for new fields ([Aschen](https://github.com/Aschen)) - [ [#1291](#1291) ] [Kuzzle CLI] Fix --help on subcommands ([Yoann-Abbes](https://github.com/Yoann-Abbes)) - [ [#1289](#1289) ] [WebSocket] Handle ping/pong packets ([scottinet](https://github.com/scottinet)) - [ [#1273](#1273) ] Fix incomplete access logs ([scottinet](https://github.com/scottinet)) ---

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)

[WebSocket] Handle ping/pong packets

b3ee6b8

scottinet added the changelog:enhancements label Apr 18, 2019

scottinet self-assigned this Apr 18, 2019

scottinet commented Apr 18, 2019

View reviewed changes

[fix] restore sinon's fake timers at the end of unit tests

2f5ec6e

ballinette reviewed Apr 18, 2019

View reviewed changes

benoitvidis reviewed Apr 18, 2019

View reviewed changes

ballinette reviewed Apr 18, 2019

View reviewed changes

ballinette approved these changes Apr 18, 2019

View reviewed changes

[review] apply @ballinette's comments

486aa88

benoitvidis approved these changes Apr 18, 2019

View reviewed changes

alexandrebouthinon approved these changes Apr 23, 2019

View reviewed changes

scottinet merged commit 885a1d3 into 1-dev Apr 24, 2019

scottinet deleted the ws-ping-pong branch April 24, 2019 08:04

Aschen mentioned this pull request Apr 25, 2019

Embedded protocols kuzzleio/documentation#286

Merged

kuzzle mentioned this pull request Apr 29, 2019

Release 1.7.3 #1293

Merged

Aschen mentioned this pull request Jun 14, 2019

Release 1.8.0 #1322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebSocket] Handle ping/pong packets #1289

[WebSocket] Handle ping/pong packets #1289

scottinet commented Apr 18, 2019 •

edited

Loading

scottinet Apr 18, 2019 •

edited

Loading

codecov-io commented Apr 18, 2019 •

edited

Loading

ballinette Apr 18, 2019 •

edited

Loading

benoitvidis left a comment

ballinette Apr 18, 2019

scottinet Apr 18, 2019

ballinette commented Apr 18, 2019 •

edited

Loading

scottinet commented Apr 18, 2019

scottinet commented Apr 18, 2019 •

edited

Loading

[WebSocket] Handle ping/pong packets #1289

[WebSocket] Handle ping/pong packets #1289

Conversation

scottinet commented Apr 18, 2019 • edited Loading

Description

Other change

How to test

scottinet Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Apr 18, 2019 • edited Loading

Codecov Report

ballinette Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

benoitvidis left a comment

Choose a reason for hiding this comment

ballinette Apr 18, 2019

Choose a reason for hiding this comment

scottinet Apr 18, 2019

Choose a reason for hiding this comment

ballinette commented Apr 18, 2019 • edited Loading

scottinet commented Apr 18, 2019

scottinet commented Apr 18, 2019 • edited Loading

scottinet commented Apr 18, 2019 •

edited

Loading

scottinet Apr 18, 2019 •

edited

Loading

codecov-io commented Apr 18, 2019 •

edited

Loading

ballinette Apr 18, 2019 •

edited

Loading

ballinette commented Apr 18, 2019 •

edited

Loading

scottinet commented Apr 18, 2019 •

edited

Loading