lib: implement sessions #289

belochub · 2017-08-01T18:19:22Z

Refs: #35

lundibundi · 2017-08-10T15:03:07Z

lib/connection.js

+      this._callbacks[messageId] = (error, sessionId) => {
+        if (!error) {
+          if (login && password) {
+            this.username = login;


Is there any need in this field (I think it's enough to have it inside Session object)?

I think, it's better to duplicate it because it doesn't use much more memory but at the same time helps to avoid long access to it, like connection.session.username.

Even though it indeed has virtually no cost, but I think that's overkill to copy it for that, you can always cache it before or use sort of accessor function to get it if you want to avoid long access.

lundibundi · 2017-08-10T15:09:44Z

lib/connection.js

-  _send(message) {
-    const data = serde.stringify(message);
-    this.transport.send(data);
+  _send(preparedMessage, message) {


Why can't we make 2 send methods _send(message) and _sendPrepared(preparedMessage) and always parse prepared message for debug mode events in the latter method with the former method only calling the latter one after preparing a message? This will avoid a lot of boilerplate code and only affect debug mode performance which is negligible.

I don't like the idea of creating the function just for stringifying messages, the output of which will always be parsed just again while sending the message, that doesn't make sense to me.

It'll only be parsed for debug messages which will not affect the production code. Also, it'll avoid passing both parsed and not parsed arguments to the _send function.

Yes, it will avoid passing both parsed and not parsed arguments to the _send method by parsing the stringified message every time, which, as I've already said, makes no sense to me.

Well it's debug mode so it's fine for me, @metarhia/jstp-core?

I am ok with both versions.

lundibundi · 2017-08-10T15:13:16Z

lib/connection.js

-  _send(message) {
-    const data = serde.stringify(message);
-    this.transport.send(data);
+  _send(preparedMessage, message) {


Also, I propose to avoid manual calling of session._bufferMessage method and instead add a boolean flag into the proposed above _sendPrepared method which will also remove some of the boilerplate code?

lundibundi · 2017-08-10T15:25:10Z

lib/transport-common.js

@@ -47,22 +49,25 @@ const newConnectFn = (
      client.application = new Application('jstp', {});
    }
    if (!client.connectPolicy) {
-      client.connectPolicy = new SimpleConnectPolicy().connect;
+      client.connectPolicy = new SimpleConnectPolicy(client.session).connect;


What is the use of the client.session parameter in SimpleConnectPolicy, ain't it better to just omit the parameters?

I forgot to change this line, thanks.

lundibundi · 2017-08-11T14:08:52Z

lib/session.js

+  constructor(connection, sessionId) {
+    super();
+
+    this._isServer = !sessionId;


Maybe just use Math.abs below? IMO it'll be cleaner.

lundibundi · 2017-08-11T14:25:24Z

lib/server.js


  this.clients = new Map();

+  this.sessions = sessionStorageProvider || new SimpleSessionStorageProvider();


Maybe sessionsProvider?

What about sessionsStorage?

lundibundi · 2017-08-11T14:29:19Z

lib/server.js

+        connection, application, strategy, credentials,
+        (error, username) => {
+          if (error || !username) {
+            callback(errors.ERR_AUTH_FAILED);


Maybe allow user to pass their own error codes? (like check if error.code and if it doesn't exist only then pass default errors.ERR_AUTH_FAILED?)

I think this has nothing to do with this PR, I didn't change this behavior.
But if you want, you can open another PR with this functionality after this one will be landed.

aqrln · 2017-08-28T11:34:14Z

lib/connection.js

@@ -200,7 +230,9 @@ class Connection extends EventEmitter {
      }
    };

-    this._send(message);
+    const preparedMessage = this._prepareMessage(message);


This pattern seems to repeat a lot. Is it worth to decouple these three lines into a method?

aqrln · 2017-08-28T12:54:41Z

lib/server.js

+      callback(null, username, session);
+    };
+
+    if (strategy !== 'anonymous') {


FWIW, you could invert this condition like so:

if (strategy === 'anonymous') { createSession(); return; }

and then reduce the level of indentation for all the rest of the function.

Not a nit though, I don't feel strongly for either way.

nechaido · 2017-11-24T14:25:37Z

@belochub please rebase on master.

belochub · 2017-11-27T18:38:27Z

@nechaido, done.

aqrln

This is not a final review, I'll look through the rest of the code today.

aqrln · 2017-11-29T03:19:38Z

lib/applications.js

@@ -18,16 +18,24 @@ const errors = require('./errors');
 //                   has the following signature
 //                   (connection, <0 or more event arguments>)
 //   version - application version of 'name' application (optional)
+//   sessionStorageProvider - provider for session storage (optional).
+//                            If provided will be used to store sessions


I feel like the subject is missing in this sentence. How about "If provided, it will..."?

aqrln · 2017-11-29T03:26:33Z

lib/applications.js

 //
 class Application {
-  constructor(name, api, eventHandlers = {}, version) {
+  constructor(name, api, eventHandlers = {}, version, sessionStorageProvider) {
+    if (typeof version === 'object') {


I'd say something like if (sessionStorageProvider === undefined && typeof version === 'object') would be more robust.

FWIW, I'm not entirely happy with the very idea of omitting some arguments in the middle of the signature, but we already have such APIs in the library which I introduced myself in the past anyway, so ¯\_(ツ)_/¯

aqrln · 2017-11-29T03:31:50Z

lib/connection.js

@@ -145,32 +143,56 @@ class Connection extends EventEmitter {
    }

    let message;
+    let isNewSession = true;
+    const handshakePayload = new Array(2);


If the whole purpose of this array is to eventually unpack it using spread operator, I'd actually find it more readable to use just two named variables (e.g., handshakeStrategy and handshakeCredentials).

aqrln · 2017-11-29T03:37:16Z

lib/connection.js


    if (process.env.NODE_ENV !== 'production') {
-      this.emit('outgoingMessage', message);
+      this.emit('outgoingMessage', message || serde.parse(preparedMessage));


serde.parse() is used directly here, but serde.stringify() is abstracted away into _prepareMessage(), which is a bit inconsistent.

nechaido · 2017-11-29T14:13:10Z

lib/server.js

+    const createSession = (username) => {
+      const session = new Session(connection);
+      if (username) {
+        session.username = username;


I'd move it to the constructor.

nechaido · 2017-11-29T15:01:05Z

lib/server.js

@@ -80,6 +81,78 @@ class Server {
    });
  }

+  startSession(connection, application, strategy, credentials, callback) {
+    const createSession = (username) => {


I'd extract it to a private function that takes (connection, application, strategy, callback) as arguments.

lundibundi · 2017-11-30T15:00:57Z

lib/connection.js

+            this.username = login;
+          }
+          this.session = new Session(this, sessionId);
+          this.session.username = this.username;


Just a nit, but maybe it'll be better to make Session's constructor get username from the connection parameter instead of assigning it here manually?

nechaido · 2017-12-14T15:44:03Z

@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>

PR-URL: #289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub · 2018-01-20T14:17:16Z

Landed in 4d600f4, fb8915c, f9b888c, and
8a0bc8b. 🎉.

PR-URL: #289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

PR-URL: #289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@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\]**

PR-URL: metarhia/jstp#289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub added lib work-in-progress labels Aug 1, 2017

belochub added this to the 1.0.0 milestone Aug 1, 2017

belochub self-assigned this Aug 1, 2017

belochub force-pushed the session-implementation branch 5 times, most recently from 0baff95 to 837a13a Compare August 4, 2017 21:22

lundibundi reviewed Aug 10, 2017

View reviewed changes

lundibundi reviewed Aug 11, 2017

View reviewed changes

aqrln reviewed Aug 28, 2017

View reviewed changes

nechaido closed this Nov 24, 2017

nechaido reopened this Nov 24, 2017

belochub force-pushed the session-implementation branch 2 times, most recently from 5553934 to 38eb903 Compare November 27, 2017 18:34

belochub removed the work-in-progress label Nov 27, 2017

belochub requested review from aqrln, lundibundi and nechaido November 27, 2017 18:38

aqrln reviewed Nov 29, 2017

View reviewed changes

nechaido reviewed Nov 29, 2017

View reviewed changes

lundibundi reviewed Nov 30, 2017

View reviewed changes

belochub force-pushed the session-implementation branch from 60cad7f to 1652fe5 Compare December 13, 2017 17:44

belochub requested review from lundibundi and nechaido December 13, 2017 17:56

nechaido approved these changes Dec 13, 2017

View reviewed changes

lundibundi approved these changes Dec 13, 2017

View reviewed changes

belochub removed the request for review from aqrln January 19, 2018 14:47

belochub and others added 4 commits January 19, 2018 17:20

lib: implement sessions

8bb03ed

test: add test for resending on connection drop

6193013

lib: fix connection restore

2ab7872

connection: use ping-pong instead of heartbeat

141bede

* 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>

belochub force-pushed the session-implementation branch from 1652fe5 to 141bede Compare January 19, 2018 15:31

belochub added a commit that referenced this pull request Jan 20, 2018

lib: implement sessions

4d600f4

PR-URL: #289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub pushed a commit that referenced this pull request Jan 20, 2018

test: add test for resending on connection drop

fb8915c

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub pushed a commit that referenced this pull request Jan 20, 2018

lib: fix connection restore

f9b888c

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub closed this Jan 20, 2018

belochub added the semver-major label Jan 22, 2018

belochub added a commit that referenced this pull request Jan 22, 2018

lib: implement sessions

85c15aa

PR-URL: #289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

test: add test for resending on connection drop

531817e

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

lib: fix connection restore

78d764f

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub added a commit that referenced this pull request Jan 22, 2018

lib: implement sessions

fd05601

PR-URL: #289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

test: add test for resending on connection drop

32d4aaa

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

lib: fix connection restore

bfe08f6

PR-URL: #289 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

belochub deleted the session-implementation branch January 24, 2018 16:44

belochub added a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018

lib: implement sessions

0ff9a21

PR-URL: metarhia/jstp#289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub added a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018

lib: implement sessions

3dbf90a

PR-URL: metarhia/jstp#289 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: implement sessions #289

lib: implement sessions #289

belochub commented Aug 1, 2017

lundibundi Aug 10, 2017

belochub Aug 11, 2017

lundibundi Aug 11, 2017

lundibundi Aug 10, 2017

belochub Aug 11, 2017

lundibundi Aug 11, 2017

belochub Aug 11, 2017

lundibundi Aug 11, 2017

nechaido Aug 11, 2017

lundibundi Aug 10, 2017

lundibundi Aug 10, 2017

belochub Aug 11, 2017

lundibundi Aug 11, 2017

lundibundi Aug 11, 2017

belochub Aug 11, 2017

lundibundi Aug 11, 2017

lundibundi Aug 11, 2017

belochub Aug 11, 2017

aqrln Aug 28, 2017

aqrln Aug 28, 2017

nechaido commented Nov 24, 2017

belochub commented Nov 27, 2017

aqrln left a comment

aqrln Nov 29, 2017

aqrln Nov 29, 2017

aqrln Nov 29, 2017

aqrln Nov 29, 2017

nechaido Nov 29, 2017

nechaido Nov 29, 2017

lundibundi Nov 30, 2017

nechaido commented Dec 14, 2017

belochub commented Jan 20, 2018 •

edited

Loading


		this.clients = new Map();

		this.sessions = sessionStorageProvider \|\| new SimpleSessionStorageProvider();

lib: implement sessions #289

lib: implement sessions #289

Conversation

belochub commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido commented Nov 24, 2017

belochub commented Nov 27, 2017

aqrln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido commented Dec 14, 2017

belochub commented Jan 20, 2018 • edited Loading

belochub commented Jan 20, 2018 •

edited

Loading