From 34998c9c069846e90949dabfb8c2e82f71d67d7a Mon Sep 17 00:00:00 2001 From: Aschen Date: Tue, 16 Jul 2019 10:06:03 +0200 Subject: [PATCH 1/5] feat: prevent promise leak on disconnect() --- doc/6/core-classes/kuzzle/disconnect/index.md | 4 +- src/protocols/abstract/common.js | 34 +++++++--- .../{query.test.js => common.test.js} | 65 +++++++++++++++---- 3 files changed, 80 insertions(+), 23 deletions(-) rename test/protocol/{query.test.js => common.test.js} (73%) diff --git a/doc/6/core-classes/kuzzle/disconnect/index.md b/doc/6/core-classes/kuzzle/disconnect/index.md index 896ab0e0b..b400a4c3f 100644 --- a/doc/6/core-classes/kuzzle/disconnect/index.md +++ b/doc/6/core-classes/kuzzle/disconnect/index.md @@ -9,7 +9,9 @@ description: Disconnect the SDK Closes the current connection to Kuzzle. The SDK then enters the `offline` state. -A call to `disconnect()` will not trigger a `disconnected` event. This event is only triggered on unexpected disconnection. +A call to `disconnect()` will not trigger a `disconnected` event. This event is only triggered on unexpected disconnection. + +If there are still pending requests during the `disconnect` call, a `discarded` event will be issued for each of them. ## Arguments diff --git a/src/protocols/abstract/common.js b/src/protocols/abstract/common.js index 93035ef8f..1ee380b45 100644 --- a/src/protocols/abstract/common.js +++ b/src/protocols/abstract/common.js @@ -5,20 +5,15 @@ const uuidv4 = require('../../uuidv4'), KuzzleEventEmitter = require('../../eventEmitter'); -// read-only properties -let - _host, - _port, - _ssl; - class AbstractWrapper extends KuzzleEventEmitter { constructor (host, options = {}) { super(); - _host = host; - _port = typeof options.port === 'number' ? options.port : 7512; - _ssl = typeof options.sslConnection === 'boolean' ? options.sslConnection : false; + this._pendingRequests = new Map(); + this._host = host; + this._port = typeof options.port === 'number' ? options.port : 7512; + this._ssl = typeof options.sslConnection === 'boolean' ? options.sslConnection : false; this.id = uuidv4(); this.state = 'offline'; @@ -46,6 +41,10 @@ class AbstractWrapper extends KuzzleEventEmitter { return this.state === 'online'; } + get pendingRequests () { + return this._pendingRequests; + } + /** * @abstract * @returns {Promise} @@ -76,6 +75,7 @@ class AbstractWrapper extends KuzzleEventEmitter { */ close () { this.state = 'offline'; + this.clear(); } query (request) { @@ -85,8 +85,12 @@ class AbstractWrapper extends KuzzleEventEmitter { Discarded request: ${JSON.stringify(request)}`)); } + this._pendingRequests.set(request.requestId, request); + return new Promise((resolve, reject) => { this.once(request.requestId, response => { + this._pendingRequests.delete(request.requestId); + if (response.error) { const error = new KuzzleError(response.error); @@ -110,6 +114,18 @@ Discarded request: ${JSON.stringify(request)}`)); return this.state === 'ready'; } + /** + * Clear pendings requests. + * Emits an event for each discarded pending request. + */ + clear () { + for (const request of this._pendingRequests.values()) { + this.emit('discarded', request); + } + + this._pendingRequests.clear(); + } + } module.exports = AbstractWrapper; diff --git a/test/protocol/query.test.js b/test/protocol/common.test.js similarity index 73% rename from test/protocol/query.test.js rename to test/protocol/common.test.js index a276cd7dd..d18020367 100644 --- a/test/protocol/query.test.js +++ b/test/protocol/common.test.js @@ -4,19 +4,16 @@ const KuzzleError = require('../../src/KuzzleError'), AbstractWrapper = require('../../src/protocols/abstract/common'); -describe('Protocol query management', () => { - describe('#query', () => { - let - protocol; - - beforeEach(function () { - protocol = new AbstractWrapper('somewhere'); - protocol.send = function(request) { - protocol.emit(request.requestId, request.response); - }; - sendSpy = sinon.spy(protocol, 'send'); - }); - +describe('Common Protocol', () => { + let + protocol; + + beforeEach(function () { + protocol = new AbstractWrapper('somewhere'); + protocol.send = function(request) { + protocol.emit(request.requestId, request.response); + }; + sendSpy = sinon.spy(protocol, 'send'); }); describe('#query', () => { @@ -50,11 +47,21 @@ describe('Protocol query management', () => { const request = {requestId: 'bar', response: {}}; protocol.query(request); + should(sendSpy) .be.calledOnce() .be.calledWith(request); }); + it('should adds the requests to pending requests', () => { + protocol.send = () => {} + const request = {requestId: 'bar', response: {}}; + + protocol.query(request); + + should(protocol.pendingRequests.get('bar')).be.eql(request); + }); + it('should fire a "queryError" event and reject if an error occurred', () => { const eventStub = sinon.stub(), @@ -103,6 +110,7 @@ describe('Protocol query management', () => { should(res.error).be.null(); should(res.result).be.exactly(response.result); should(res.status).be.exactly(42); + should(protocol.pendingRequests.get('bar')).be.undefined(); }); }); @@ -151,6 +159,37 @@ describe('Protocol query management', () => { should(error.count).eql(42); }); }); + }); + + describe('#clear', () => { + it('should send "discarded" event for each pending request', () => { + const + request1 = { requestId: '12345', body: 'foobar' }, + request2 = { requestId: '54321', body: 'barfoo' }; + + protocol._pendingRequests.set(request1, request1); + protocol._pendingRequests.set(request2, request2); + + const listener = sinon.stub(); + protocol.on('discarded', listener); + + protocol.clear(); + + + should(listener).be.calledTwice(); + should(listener.getCall(0).args).be.eql([request1]); + should(listener.getCall(1).args).be.eql([request2]); + }); + }); + + describe('#close', () => { + it('should set state to "offline" and clear pending requests', () => { + protocol.clear = sinon.stub(); + + protocol.close(); + should(protocol.state).be.eql('offline'); + should(protocol.clear).be.calledOnce(); + }) }); }); From c7becd687f730a20be3a3f08143b4fa64d65aff8 Mon Sep 17 00:00:00 2001 From: Aschen Date: Tue, 16 Jul 2019 10:51:58 +0200 Subject: [PATCH 2/5] feat: clear on disconnected event --- src/protocols/abstract/common.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/protocols/abstract/common.js b/src/protocols/abstract/common.js index 1ee380b45..9ff172380 100644 --- a/src/protocols/abstract/common.js +++ b/src/protocols/abstract/common.js @@ -23,6 +23,8 @@ class AbstractWrapper extends KuzzleEventEmitter { this[opt] = options[opt]; } }); + + this.on('disconnected', () => this.clear()); } get host () { From 60ddf4b487ca1836b0a60cb80ce0079c998c1b67 Mon Sep 17 00:00:00 2001 From: Aschen Date: Tue, 16 Jul 2019 11:26:14 +0200 Subject: [PATCH 3/5] fix tests --- src/protocols/abstract/common.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/protocols/abstract/common.js b/src/protocols/abstract/common.js index 9ff172380..e8ab0ebda 100644 --- a/src/protocols/abstract/common.js +++ b/src/protocols/abstract/common.js @@ -28,15 +28,15 @@ class AbstractWrapper extends KuzzleEventEmitter { } get host () { - return _host; + return this._host; } get port () { - return _port; + return this._port; } get ssl () { - return _ssl; + return this._ssl; } get connected () { From 906655fc2362f58b1ff4d2dbe80331c39e9057bd Mon Sep 17 00:00:00 2001 From: Aschen Date: Tue, 16 Jul 2019 13:03:41 +0200 Subject: [PATCH 4/5] fix tests --- src/protocols/abstract/common.js | 4 ++-- test/protocol/common.test.js | 14 +++----------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/protocols/abstract/common.js b/src/protocols/abstract/common.js index e8ab0ebda..7674b4648 100644 --- a/src/protocols/abstract/common.js +++ b/src/protocols/abstract/common.js @@ -23,8 +23,6 @@ class AbstractWrapper extends KuzzleEventEmitter { this[opt] = options[opt]; } }); - - this.on('disconnected', () => this.clear()); } get host () { @@ -68,6 +66,8 @@ class AbstractWrapper extends KuzzleEventEmitter { * Called when the client's connection is established */ clientConnected (state, wasConnected) { + this.on('disconnected', () => this.clear()); + this.state = state || 'ready'; this.emit(wasConnected && 'reconnect' || 'connect'); } diff --git a/test/protocol/common.test.js b/test/protocol/common.test.js index d18020367..385072921 100644 --- a/test/protocol/common.test.js +++ b/test/protocol/common.test.js @@ -6,6 +6,7 @@ const describe('Common Protocol', () => { let + sendSpy, protocol; beforeEach(function () { @@ -17,17 +18,9 @@ describe('Common Protocol', () => { }); describe('#query', () => { - let - sendSpy, - protocol; beforeEach(() => { - protocol = new AbstractWrapper('somewhere'); protocol.isReady = sinon.stub().returns(true); - protocol.send = function(request) { - protocol.emit(request.requestId, request.response); - }; - sendSpy = sinon.spy(protocol, 'send'); protocol._emitRequest = sinon.stub().resolves(); }); @@ -54,7 +47,7 @@ describe('Common Protocol', () => { }); it('should adds the requests to pending requests', () => { - protocol.send = () => {} + protocol.send = () => {}; const request = {requestId: 'bar', response: {}}; protocol.query(request); @@ -175,7 +168,6 @@ describe('Common Protocol', () => { protocol.clear(); - should(listener).be.calledTwice(); should(listener.getCall(0).args).be.eql([request1]); should(listener.getCall(1).args).be.eql([request2]); @@ -190,6 +182,6 @@ describe('Common Protocol', () => { should(protocol.state).be.eql('offline'); should(protocol.clear).be.calledOnce(); - }) + }); }); }); From b37610ac216bce0b5fbc889cf229f7923524983c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Cottinet?= Date: Wed, 17 Jul 2019 17:20:51 +0200 Subject: [PATCH 5/5] [fix] typo --- test/protocol/common.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/protocol/common.test.js b/test/protocol/common.test.js index 385072921..87c3efed6 100644 --- a/test/protocol/common.test.js +++ b/test/protocol/common.test.js @@ -46,7 +46,7 @@ describe('Common Protocol', () => { .be.calledWith(request); }); - it('should adds the requests to pending requests', () => { + it('should add the requests to pending requests', () => { protocol.send = () => {}; const request = {requestId: 'bar', response: {}};