lib: optimize connection events #222

lundibundi · 2017-06-16T17:39:51Z

remove unnecessary events like 'handshake', 'call', 'handshakeRequest'
replace 'callbackForUnknownPacket' with rejectPacket
change signature of connection 'event' (remote event), now it contains
interfaceName, eventName, eventArgs

Also event though state packet event is now removed it has to be put back with proper data inside the event, because _emitPacketEvent it used adds a lot of redundant data. Can you please comment on which data should be included in state event?
UPD: as discused state packets are removed for now.

Fixes: #49

belochub · 2017-06-16T21:17:35Z

I think that it is better to add logger functionality in a different PR or at least in a different commit in this PR because it isn't really connected to connection events optimization.

belochub · 2017-06-16T21:26:54Z

Also, it would be great to have an ability to add multiple loggers instead of setting just one.

lundibundi · 2017-06-16T21:31:10Z

@belochub yeah, different commit I guess even though the change is good enough to be in a different PR I think there is no need in that because that PR will not have any actual usages of logger, so even if you set the logger it will do nothing, but having a different PR is fine with me too.

Could you elaborate on the latter comment, what multiple loggers can you have?

belochub · 2017-06-16T21:53:32Z

I meant that you can create a new PR based on this PR with logger functionality.

About multiple loggers, there is a lot of possible variants, for example, one logger for console output, another for file logging and the third for logging over the network. Of course, it can be implemented even with the current changes, but it will be much easier if you add a possibility to add multiple loggers.

lundibundi · 2017-06-16T21:59:00Z

@belochub I do not see any reason to implement it here, I think that's the job of a logging framework user decides to use with our project, the logger I add here is just an api (at least winston and bunyan can do that pretty good).

belochub · 2017-06-16T22:09:43Z

Yeah, but it will do no harm if you add the possibility to have multiple loggers. It will make it possible to use small and simple self-written loggers instead of bigger frameworks, and you can still use the frameworks by adding just one logger instance.

lundibundi · 2017-06-17T09:22:43Z

As discussed, waiting for #224.

* remove unnecessary events like 'handshake', 'call', 'handshakeRequest' * replace 'callbackForUnknownPacket' with rejectPacket * change signature of connection 'event' (remote event), now it contains interfaceName, eventName, eventArgs Fixes: #49

lundibundi · 2017-06-17T12:15:25Z

Properly split changes, removing blocked.

aqrln

LGTM with nits.

aqrln · 2017-06-19T12:26:19Z

lib/connection.js

@@ -58,6 +58,10 @@ function Connection(transport, server, client) {

  this._heartbeatCallbackInstance = null;

+  this._emitError = (error) => {


Can you please add a comment explaining the reason of this being created in the constructor instead of on the prototype? It is not immediately obvious until one reads the callMethod() code.

aqrln · 2017-06-19T12:31:11Z

test/node/connection-event.js

-      test.strictEqual(event.interfaceName, iface,
+    server.getClients()[0].on('event',
+      (interfaceName, remoteName, remoteArgs) => {
+        test.strictEqual(interfaceName, iface,
        'event interface must match');


Nit: can you please indent this line?

Sure. We should find a way to track it automatically =).

It will be tracked automatically as soon as we update ESLint to 4.0. The only reason I haven't done it yet is that we use the eslint-plugin-import plugin which still has eslint@3.x as its peerDependency (though it seems to work with the latest eslint nevertheless).

aqrln · 2017-06-19T12:31:19Z

test/node/connection-event.js

        'event interface must match');
-      test.strictEqual(event.remoteEventName, eventName,
+        test.strictEqual(remoteName, eventName,
        'event name must be equal to the emitted one');


aqrln · 2017-06-19T12:31:25Z

test/node/connection-event.js

        'event name must be equal to the emitted one');
-      test.strictDeepEqual(event.remoteEventArgs, args,
+        test.strictDeepEqual(remoteArgs, args,
        'event arguments must be equal to the passed ones');


aqrln · 2017-06-19T12:31:48Z

test/node/connection-event.js

-        test.strictEqual(event.interfaceName, iface,
+      server.getClients()[0].on('event',
+        (interfaceName, remoteName, remoteArgs) => {
+          test.strictEqual(interfaceName, iface,
          'event interface must match');


aqrln · 2017-06-19T12:31:53Z

test/node/connection-event.js

          'event interface must match');
-        test.strictEqual(event.remoteEventName, eventName,
+          test.strictEqual(remoteName, eventName,
          'event name must be equal to the emitted one');


aqrln · 2017-06-19T12:32:01Z

test/node/connection-event.js

          'event name must be equal to the emitted one');
-        test.strictDeepEqual(event.remoteEventArgs, args,
+          test.strictDeepEqual(remoteArgs, args,
          'event arguments must be equal to the passed ones');


belochub

LGTM with a small nit: this PR doesn't really "optimize" connection events, it just avoids emitting all of the unused events. May you please update the name and commit message accordingly?

nechaido

LGTM pending requested changes.

lundibundi · 2017-06-20T11:56:00Z

@belochub it also reuses same error callback, removes unneded data from jstp event and replaces callbackForUnknownPacket with 'rejectPacket', should we really just call this one 'remove redundant events'?

belochub · 2017-06-20T12:31:29Z

@lundibundi, I didn't say that we should call it "remove redundant events".

lundibundi · 2017-06-20T12:46:19Z

@belochub I just suggested a name that will present that this PR removes those events, what is your suggestion?

belochub · 2017-06-20T13:31:47Z

@lundibundi, I am sorry, I misunderstood your comment, to me it looked like you disagree with the idea of renaming this at all.
In my opinion, a better name for this could be "avoid emitting unused events", but it isn't perfect as well.

aqrln · 2017-06-21T06:52:24Z

The current commit message is fine since the PR does not only vanish redundant events, but also performs various other performance-related optimization (e.g., avoiding creation of extra closure in order to emit 'error' event in callMethod()) and the removal of unnecessary events is explicitly stated in the full commit message.

* Remove unnecessary events like 'handshake', 'call', 'handshakeRequest'. * Replace 'callbackForUnknownPacket' with _rejectPacket(). * Change signature of connection 'event' (remote event), now it contains interfaceName, eventName, eventArgs. * Avoid creating extra closures to conditionally emit the 'error' event in callbacks instantiated inside callPacket(). Fixes: #49 PR-URL: #222 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Dmytro Nechai <nechaido@gmail.com>

aqrln · 2017-06-21T06:56:42Z

Landed in 637bb40.

aqrln · 2017-06-21T06:57:45Z

@belochub please let me know if I was too fast on this and I should have waited for your reply.

* Remove unnecessary events like 'handshake', 'call', 'handshakeRequest'. * Replace 'callbackForUnknownPacket' with _rejectPacket(). * Change signature of connection 'event' (remote event), now it contains interfaceName, eventName, eventArgs. * Avoid creating extra closures to conditionally emit the 'error' event in callbacks instantiated inside callPacket(). Fixes: #49 PR-URL: #222 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Dmytro Nechai <nechaido@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\]**

lundibundi added connection optimization semver-major test labels Jun 16, 2017

lundibundi added this to the 1.0.0 milestone Jun 16, 2017

lundibundi self-assigned this Jun 16, 2017

lundibundi requested review from aqrln and nechaido June 16, 2017 17:39

lundibundi added the blocked label Jun 17, 2017

lib: optimize connection events

a972955

* remove unnecessary events like 'handshake', 'call', 'handshakeRequest' * replace 'callbackForUnknownPacket' with rejectPacket * change signature of connection 'event' (remote event), now it contains interfaceName, eventName, eventArgs Fixes: #49

lundibundi force-pushed the optimize-connection-events branch from a9f1418 to a972955 Compare June 17, 2017 12:13

lundibundi removed the blocked label Jun 17, 2017

fixup! lib: remove state packets

1789dd8

aqrln approved these changes Jun 19, 2017

View reviewed changes

aqrln removed the test label Jun 19, 2017

belochub approved these changes Jun 19, 2017

View reviewed changes

nechaido approved these changes Jun 20, 2017

View reviewed changes

fixup! add requested changes

91087d3

aqrln closed this Jun 21, 2017

aqrln deleted the optimize-connection-events branch June 21, 2017 06:56

aqrln mentioned this pull request Jul 10, 2017

Emit events about network messages on connections #250

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

lib: optimize connection events #222

lib: optimize connection events #222

lundibundi commented Jun 16, 2017 •

edited

Loading

belochub commented Jun 16, 2017

belochub commented Jun 16, 2017

lundibundi commented Jun 16, 2017 •

edited

Loading

belochub commented Jun 16, 2017

lundibundi commented Jun 16, 2017

belochub commented Jun 16, 2017

lundibundi commented Jun 17, 2017

lundibundi commented Jun 17, 2017

aqrln left a comment

aqrln Jun 19, 2017

aqrln Jun 19, 2017

lundibundi Jun 20, 2017

aqrln Jun 20, 2017

aqrln Jun 19, 2017

aqrln Jun 19, 2017

aqrln Jun 19, 2017

aqrln Jun 19, 2017

aqrln Jun 19, 2017

belochub left a comment

nechaido left a comment

lundibundi commented Jun 20, 2017

belochub commented Jun 20, 2017

lundibundi commented Jun 20, 2017

belochub commented Jun 20, 2017

aqrln commented Jun 21, 2017

aqrln commented Jun 21, 2017

aqrln commented Jun 21, 2017

		@@ -58,6 +58,10 @@ function Connection(transport, server, client) {

		this._heartbeatCallbackInstance = null;

		this._emitError = (error) => {

lib: optimize connection events #222

lib: optimize connection events #222

Conversation

lundibundi commented Jun 16, 2017 • edited Loading

belochub commented Jun 16, 2017

belochub commented Jun 16, 2017

lundibundi commented Jun 16, 2017 • edited Loading

belochub commented Jun 16, 2017

lundibundi commented Jun 16, 2017

belochub commented Jun 16, 2017

lundibundi commented Jun 17, 2017

lundibundi commented Jun 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub left a comment

Choose a reason for hiding this comment

nechaido left a comment

Choose a reason for hiding this comment

lundibundi commented Jun 20, 2017

belochub commented Jun 20, 2017

lundibundi commented Jun 20, 2017

belochub commented Jun 20, 2017

aqrln commented Jun 21, 2017

aqrln commented Jun 21, 2017

aqrln commented Jun 21, 2017

lundibundi commented Jun 16, 2017 •

edited

Loading

lundibundi commented Jun 16, 2017 •

edited

Loading