connection: use ping-pong instead of heartbeat #303

nechaido · 2017-11-22T14:59:15Z

Send Ping-Pong instead of Heartbeat messages.
Use setInterval() instead of timers.enroll().
Rewrite tests.

Fixes: #217

* Send Ping-Pong instead of Heartbeat messages. * Use setInterval() instead of timers.enroll(). * Rewrite tests.

aqrln · 2017-11-22T15:18:37Z

lib/connection.js

@@ -241,33 +237,36 @@ class Connection extends EventEmitter {
  // Send a pong message
  //
  pong(messageId) {
+    if (this._closed) {


Given that it's a public method, I'd say that the caller shouldn't invoke pong() if the connection is closed. Otherwise, this check makes pong() inconsistent with other methods of a connection in a way that one may call it on a closed connection without any errors, which doesn't sound right.

@aqrln mark pong as a private method, there is no reason for it to be public.

@nechaido sounds good to me, but the same goes for callback, then. (Though it's kinda out of scope of this PR.)

UPD: #306

belochub · 2017-11-22T17:28:22Z

lib/connection.js

@@ -53,18 +52,15 @@ class Connection extends EventEmitter {
    this.application = null;
    this.remoteProxies = {};

-    this._heartbeatCallbackInstance = null;
+    this._heartbeatInterval = null;


I think it will be more appropriate to call this field _heartbeatTimer or _heartbeatTimeout because it is somewhat confusing to use interval here, due to the same word being used as a parameter name in startHeartbeat method and having the meaning of the delay between heartbeats.

belochub · 2017-11-22T17:38:29Z

lib/connection.js

-      if (process.env.NODE_ENV !== 'production') {
-        this.emit('heartbeat', this._heartbeatMessage);
+    const heartbeat = () => {
+      if (this._closed) {


This can be simplified to

if (!this._closed) { this.ping(); }

belochub · 2017-11-22T17:40:58Z

lib/connection.js

@@ -400,6 +374,7 @@ class Connection extends EventEmitter {
  // Closed socket event handler
  //
  _onSocketClose() {
+    this.closed = true;


Looks like there is a typo here, it should be this._closed instead of this.closed.

belochub · 2017-11-22T19:12:45Z

There is no need to add test label to this PR, it is similar to the situation in #190 (comment).

aqrln · 2017-11-22T20:15:13Z

There is no need to add test label to this PR, it is similar to the situation in #190 (comment).

Yup, not even lib. The subsystem should be just connection.

nechaido · 2017-11-23T18:40:17Z

@belochub @aqrln ping.

* Send Ping-Pong instead of Heartbeat messages. * Use setInterval() instead of timers.enroll(). * Rewrite tests. Fixes: #217 PR-URL: #303 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

nechaido · 2017-11-24T08:56:48Z

Landed in 37918fe.

* Send Ping-Pong instead of Heartbeat messages. * Use setInterval() instead of timers.enroll(). * Rewrite tests. Fixes: #217 PR-URL: #303 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) * **parser:** fix a possible memory leak *(Alexey Orlenko)* [#44](#44) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** make parser single-pass *(Mykola Bilochub)* [#61](#61) * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **parser:** fix bug causing node to crash *(Mykola Bilochub)* [#75](#75) * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **test:** add JSON5 specs test suite *(Alexey Orlenko)* [#158](#158) * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **parser:** implement NaN and Infinity parsing *(Mykola Bilochub)* [#201](#201) * **parser:** improve string parsing performance *(Mykola Bilochub)* [#220](#220) * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **lib:** implement API versioning *(Denys Otrishko)* [#231](#231) **\[semver-minor\]** * **lib:** allow to set event handlers in application *(Denys Otrishko)* [#286](#286) **\[semver-minor\]** * **lib:** allow to broadcast events from server *(Denys Otrishko)* [#287](#287) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

lib, test: use ping-pong instead of heartbeat

670d725

* Send Ping-Pong instead of Heartbeat messages. * Use setInterval() instead of timers.enroll(). * Rewrite tests.

nechaido added the lib label Nov 22, 2017

aqrln added connection protocol and removed lib labels Nov 22, 2017

nechaido requested a review from belochub November 22, 2017 15:04

aqrln reviewed Nov 22, 2017

View reviewed changes

nechaido force-pushed the use-ping-pong-instead-of-heartbeat branch 2 times, most recently from c93dfc0 to b9f023c Compare November 22, 2017 16:21

fixup! lib, test: use ping-pong instead of heartbeat

ec82ee5

nechaido force-pushed the use-ping-pong-instead-of-heartbeat branch from b9f023c to ec82ee5 Compare November 22, 2017 16:23

aqrln added the semver-major label Nov 22, 2017

aqrln mentioned this pull request Nov 22, 2017

connection: make callback method private #306

Closed

belochub reviewed Nov 22, 2017

View reviewed changes

fixup! lib, test: use ping-pong instead of heartbeat

30e07de

nechaido changed the title ~~lib, test: use ping-pong instead of heartbeat~~ lib: use ping-pong instead of heartbeat Nov 22, 2017

nechaido changed the title ~~lib: use ping-pong instead of heartbeat~~ connection: use ping-pong instead of heartbeat Nov 22, 2017

aqrln approved these changes Nov 23, 2017

View reviewed changes

belochub approved these changes Nov 23, 2017

View reviewed changes

nechaido closed this Nov 24, 2017

nechaido deleted the use-ping-pong-instead-of-heartbeat branch November 24, 2017 08:56

belochub mentioned this pull request Dec 7, 2017

lib: implement sessions #289

Closed

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connection: use ping-pong instead of heartbeat #303

connection: use ping-pong instead of heartbeat #303

nechaido commented Nov 22, 2017 •

edited

Loading

aqrln Nov 22, 2017

nechaido Nov 22, 2017

aqrln Nov 22, 2017 •

edited

Loading

belochub Nov 22, 2017

belochub Nov 22, 2017

belochub Nov 22, 2017

belochub commented Nov 22, 2017

aqrln commented Nov 22, 2017

nechaido commented Nov 23, 2017

nechaido commented Nov 24, 2017

connection: use ping-pong instead of heartbeat #303

connection: use ping-pong instead of heartbeat #303

Conversation

nechaido commented Nov 22, 2017 • edited Loading

aqrln Nov 22, 2017

Choose a reason for hiding this comment

nechaido Nov 22, 2017

Choose a reason for hiding this comment

aqrln Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

belochub Nov 22, 2017

Choose a reason for hiding this comment

belochub Nov 22, 2017

Choose a reason for hiding this comment

belochub Nov 22, 2017

Choose a reason for hiding this comment

belochub commented Nov 22, 2017

aqrln commented Nov 22, 2017

nechaido commented Nov 23, 2017

nechaido commented Nov 24, 2017

nechaido commented Nov 22, 2017 •

edited

Loading

aqrln Nov 22, 2017 •

edited

Loading