Fix disabled protocol initialization #1298

Aschen · 2019-05-09T15:47:58Z

What does this PR do ?

Even with the disabled property set to false, the protocols are all loaded into memory.
The footprint is small but I ran into a bug with realtime notification where Kuzzle try to send notification with the dispatch method even if the protocol was disabled.

Now the protocols are initialized and if they are disabled Kuzzle does not keep them in memory.

How should this be manually tested?

Step 1 : Subscribe to realtime notification with MQTT disabled
Step 2 : When a notification arrive, you should'nt see an error message in the console

Other changes

Protocol init method now return a Promise
Rename Protocol.protocol property to Protocol.name

codecov · 2019-05-09T16:20:05Z

Codecov Report

Merging #1298 into 1-dev will decrease coverage by 0.01%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##            1-dev    #1298      +/-   ##
==========================================
- Coverage   93.83%   93.82%   -0.02%     
==========================================
  Files          98       98              
  Lines        6848     6865      +17     
==========================================
+ Hits         6426     6441      +15     
- Misses        422      424       +2

Impacted Files	Coverage Δ
...pi/core/entrypoints/embedded/protocols/socketio.js	`93.15% <100%> (+0.09%)`	⬆️
lib/services/broker/index.js	`100% <100%> (ø)`	⬆️
lib/api/core/httpRouter/index.js	`98.68% <100%> (ø)`	⬆️
...i/core/entrypoints/embedded/protocols/websocket.js	`93.07% <100%> (ø)`	⬆️
...ib/api/core/entrypoints/embedded/protocols/mqtt.js	`95.23% <100%> (+1.12%)`	⬆️
lib/api/core/entrypoints/embedded/index.js	`98.31% <90%> (-0.53%)`	⬇️
...pi/core/entrypoints/embedded/protocols/protocol.js	`82.35% <90.9%> (+2.35%)`	⬆️
...ib/api/core/entrypoints/embedded/protocols/http.js	`99.33% <97.87%> (-0.67%)`	⬇️

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 bbe7042...0fa800a. Read the comment docs.

codecov · 2019-05-09T16:20:07Z

Codecov Report

Merging #1298 into 1-dev will decrease coverage by 0.01%.
The diff coverage is 97.58%.

@@            Coverage Diff             @@
##            1-dev    #1298      +/-   ##
==========================================
- Coverage   93.85%   93.83%   -0.02%     
==========================================
  Files          98       98              
  Lines        6862     6879      +17     
==========================================
+ Hits         6440     6455      +15     
- Misses        422      424       +2

Impacted Files	Coverage Δ
...pi/core/entrypoints/embedded/protocols/socketio.js	`93.15% <100%> (+0.09%)`	⬆️
lib/services/broker/index.js	`100% <100%> (ø)`	⬆️
lib/api/core/httpRouter/index.js	`98.68% <100%> (ø)`	⬆️
...i/core/entrypoints/embedded/protocols/websocket.js	`93.07% <100%> (ø)`	⬆️
...ib/api/core/entrypoints/embedded/protocols/mqtt.js	`95.23% <100%> (+1.12%)`	⬆️
lib/api/core/entrypoints/embedded/index.js	`98.31% <90%> (-0.53%)`	⬇️
...pi/core/entrypoints/embedded/protocols/protocol.js	`82.35% <90.9%> (+2.35%)`	⬆️
...ib/api/core/entrypoints/embedded/protocols/http.js	`99.33% <97.91%> (-0.67%)`	⬇️

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 363d1da...21c821c. Read the comment docs.

scottinet

Also, you have to unstage the package-lock file

scottinet · 2019-05-10T07:09:47Z

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

@@ -191,6 +200,9 @@ class EmbeddedEntryPoint extends EntryPoint {
        })
        .timeout(this.kuzzle.config.services.common.defaultInitTimeout)
        .then(() => {
+          if (this.protocols[manifest.name]) {
+            throw new KuzzleInternalError(`Conflicting protocol name "${manifest.name}".`);


Should be a PluginImplementationError error

scottinet · 2019-05-10T07:12:02Z

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

@@ -374,7 +382,7 @@ class HttpProtocol extends Protocol {
      };

    if (typeof allowCompression !== 'boolean') {
-      throw new Error('Invalid HTTP "allowCompression" parameter value: expected a boolean value');
+      throw new KuzzleInternalError('Invalid HTTP "allowCompression" parameter value: expected a boolean value');


(discussion - this doesn't block this PR)

No, this is not a Kuzzle internal error, but a configuration error (which is probably a lacking Error object).

This doesn't really matter because, in the end, the error is caught during initialization, and that aborts Kuzzle's start sequence. The error type is only really useful when a response is sent back to a client, or when it's a plugin error.

Actually I opted for a different approach myself, while working on you-know-what: I'm using the assert native module for checks made during startup.
It's cleaner as in most cases we expect failed assertions to stop the program execution at some point.

I agree with the usage of assert

scottinet · 2019-05-10T07:18:49Z

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

@@ -112,6 +136,11 @@ describe('lib/core/api/core/entrypoints/embedded/index', () => {
        error: sinon.spy()
      }
    });
+
+    HttpMock.prototype.init = sinon.stub().resolves(true);


DRY: putting this.init = sinon.stub().resolves(true); in the FakeProtocol constructor allows you to get rid of these lines

No because I want to know separately when I call one of the init function

I don't understand: with my proposal, won't each class instance have its own init function?

Since I am instantiating theses classes in the EmbeddedEntrypoint, I have to set the stub directly on the prototype if I want to be able to change the returned value (eg: MqttMock.prototype.init = sinon.stub().resolves(false);)

scottinet · 2019-05-10T07:21:56Z

test/api/core/entrypoints/embedded/protocols/mqtt.test.js

+    it('should return false if the protocol is disabled', () => {
+      entrypoint.config.protocols.mqtt.enabled = false;
+
+      return protocol.init(entrypoint)


(nitpicking) can be simplified with fulfilledWith:

return should(protocol.init(entrypoint)).fulfilledWith(false);

…le into fix-disabled-protocol-init

alexandrebouthinon · 2019-05-20T09:40:41Z

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


-    for (const protocol of Object.keys(this.protocols)) {
-      this.protocols[protocol].init(this);
+    for (const ProtocolClass of [


Are you sure removing this.protocols property does not break the custom protocols implementation capability?

this.protocols is not removed
What this PR does is to go back to a previous state where only active embedded protocols are stored in that object (I changed that not that long ago, when even inactive protocols where stored)

Ok tanks for those explanations 🙂

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)

Aschen added the changelog:bug-fixes label May 9, 2019

Aschen self-assigned this May 9, 2019

scottinet suggested changes May 10, 2019

View reviewed changes

Aschen force-pushed the fix-disabled-protocol-init branch from 256cee0 to 3cf1a64 Compare May 10, 2019 11:56

Dont load disabled protocols

ae349dd

Aschen force-pushed the fix-disabled-protocol-init branch from 3cf1a64 to ae349dd Compare May 10, 2019 11:57

Aschen and others added 3 commits May 10, 2019 13:58

Merge branch '1-dev' into fix-disabled-protocol-init

f3fb1f6

fix tests

5817e18

Merge branch '1-dev' into fix-disabled-protocol-init

61e3425

scottinet approved these changes May 14, 2019

View reviewed changes

Merge branch 'fix-disabled-protocol-init' of github.com:kuzzleio/kuzz…

9fc69c2

…le into fix-disabled-protocol-init

benoitvidis approved these changes May 15, 2019

View reviewed changes

Merge branch '1-dev' into fix-disabled-protocol-init

782fc5f

scottinet mentioned this pull request May 16, 2019

Upgrade the WebSocket libraries #1301

Merged

[bug] fix inverted assert logic

18fa313

alexandrebouthinon reviewed May 20, 2019

View reviewed changes

Merge branch '1-dev' into fix-disabled-protocol-init

21c821c

alexandrebouthinon approved these changes May 20, 2019

View reviewed changes

alexandrebouthinon merged commit 5e8dc8b into 1-dev May 20, 2019

alexandrebouthinon deleted the fix-disabled-protocol-init branch May 20, 2019 11:37

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

Fix disabled protocol initialization #1298

Fix disabled protocol initialization #1298

Aschen commented May 9, 2019 •

edited

Loading

codecov bot commented May 9, 2019

codecov bot commented May 9, 2019 •

edited

Loading

scottinet left a comment

scottinet May 10, 2019

scottinet May 10, 2019 •

edited

Loading

scottinet May 10, 2019

Aschen May 10, 2019

scottinet May 10, 2019

Aschen May 10, 2019

scottinet May 10, 2019

Aschen May 10, 2019

scottinet May 10, 2019

alexandrebouthinon May 20, 2019

scottinet May 20, 2019

alexandrebouthinon May 20, 2019

Fix disabled protocol initialization #1298

Fix disabled protocol initialization #1298

Conversation

Aschen commented May 9, 2019 • edited Loading

What does this PR do ?

How should this be manually tested?

Other changes

codecov bot commented May 9, 2019

Codecov Report

codecov bot commented May 9, 2019 • edited Loading

Codecov Report

scottinet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottinet May 10, 2019 • edited Loading

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

Aschen commented May 9, 2019 •

edited

Loading

codecov bot commented May 9, 2019 •

edited

Loading

scottinet May 10, 2019 •

edited

Loading