From e07656b233548426e8eded2c86f8cc2144401e02 Mon Sep 17 00:00:00 2001 From: Philippe Seewer Date: Fri, 7 May 2021 14:04:37 +0200 Subject: [PATCH 1/5] SearchPager: Implement queueing events until a listener appears MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the same problems for paged searches as does https://github.com/ldapjs/node-ldapjs/pull/610 for unpaged searches. By passing an EventEmitter via callback there exist cases when events are emitted before listeners are registered, resulting in missed events. The Change turns SearchPager into a CorkedEmitter which is already used as a solution for non paged searches. Doing so requires the internal 'search' event to be dropped. This change adapts a test case originally by László Szűcs (@ifroz). Signed-off-by: Philippe Seewer --- lib/client/client.js | 4 ++-- lib/client/search_pager.js | 12 +++++++----- test/client.test.js | 21 +++++++++++++++++++++ 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/lib/client/client.js b/lib/client/client.js index bdff844..f54cf6a 100644 --- a/lib/client/client.js +++ b/lib/client/client.js @@ -630,9 +630,9 @@ Client.prototype.search = function search (base, callback: callback, controls: controls, pageSize: size, - pagePause: pageOpts.pagePause + pagePause: pageOpts.pagePause, + sendRequest: sendRequest }) - pager.on('search', sendRequest) pager.begin() } else { sendRequest(controls, new CorkedEmitter(), callback) diff --git a/lib/client/search_pager.js b/lib/client/search_pager.js index d44f318..83a77e9 100644 --- a/lib/client/search_pager.js +++ b/lib/client/search_pager.js @@ -10,6 +10,8 @@ const assert = require('assert-plus') // var Protocol = require('../protocol') const PagedControl = require('../controls/paged_results_control.js') +const CorkedEmitter = require('../corked_emitter.js') + /// --- API /** @@ -29,19 +31,20 @@ const PagedControl = require('../controls/paged_results_control.js') * will be emitted (and 'end' will not be). By listening to * 'pageError', a successful search that lacks paging will be * able to emit 'end'. - * 3. search - Emitted as an internal event to trigger another client search. */ function SearchPager (opts) { assert.object(opts) assert.func(opts.callback) assert.number(opts.pageSize) + assert.func(opts.sendRequest) - EventEmitter.call(this, {}) + CorkedEmitter.call(this, {}) this.callback = opts.callback this.controls = opts.controls this.pageSize = opts.pageSize this.pagePause = opts.pagePause + this.sendRequest = opts.sendRequest this.controls.forEach(function (control) { if (control.type === PagedControl.OID) { @@ -60,7 +63,7 @@ function SearchPager (opts) { emitter.on('error', this._onError.bind(this)) this.childEmitter = emitter } -util.inherits(SearchPager, EventEmitter) +util.inherits(SearchPager, CorkedEmitter) module.exports = SearchPager /** @@ -143,8 +146,7 @@ SearchPager.prototype._nextPage = function _nextPage (cookie) { } })) - this.emit('search', controls, this.childEmitter, - this._sendCallback.bind(this)) + this.sendRequest(controls, this.childEmitter, this._sendCallback.bind(this)) } /** diff --git a/test/client.test.js b/test/client.test.js index 8fe6194..f95e4f7 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -899,6 +899,27 @@ tap.test('search paged', { timeout: 10000 }, function (t) { }) }) + tap.test('paged - search with delayed event listener binding', function (t) { + t.context.client.search('cn=paged', { filter: '(objectclass=*)', paged : true }, function (err, res) { + t.error(err) + setTimeout(() => { + let gotEntry = 0 + console.log('registering searchEntry'); + + res.on('searchEntry', function () { + gotEntry++ + }) + res.on('error', function (err) { + t.fail(err) + }) + res.on('end', function () { + t.equal(gotEntry, 1000) + t.end() + }) + }, 100) + }) + }) + t.end() }) From d064b359d32759cd15dec715ffb1778f3145f3dd Mon Sep 17 00:00:00 2001 From: Philippe Seewer Date: Fri, 7 May 2021 15:50:02 +0200 Subject: [PATCH 2/5] Run lint. Signed-off-by: Philippe Seewer --- test/client.test.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/client.test.js b/test/client.test.js index f95e4f7..1b69454 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -900,16 +900,14 @@ tap.test('search paged', { timeout: 10000 }, function (t) { }) tap.test('paged - search with delayed event listener binding', function (t) { - t.context.client.search('cn=paged', { filter: '(objectclass=*)', paged : true }, function (err, res) { + t.context.client.search('cn=paged', { filter: '(objectclass=*)', paged: true }, function (err, res) { t.error(err) setTimeout(() => { - let gotEntry = 0 - console.log('registering searchEntry'); - + let gotEntry = 0 res.on('searchEntry', function () { gotEntry++ }) - res.on('error', function (err) { + res.on('error', function (err) { t.fail(err) }) res.on('end', function () { From e74ba91238d4fec511462684371b9cad80dceb12 Mon Sep 17 00:00:00 2001 From: zhaoyunfeng Date: Mon, 24 May 2021 12:34:32 +0800 Subject: [PATCH 3/5] feat: now search callback response will emit SearchRequest after every search request is sent --- docs/client.md | 21 ++++++++++++--------- lib/client/client.js | 4 +++- lib/client/search_pager.js | 1 + test/client.test.js | 16 ++++++++++++++-- 4 files changed, 30 insertions(+), 12 deletions(-) diff --git a/docs/client.md b/docs/client.md index c267dab..60666f9 100644 --- a/docs/client.md +++ b/docs/client.md @@ -276,15 +276,15 @@ containing the following fields: |timeLimit |the maximum amount of time the server should take in responding, in seconds. Defaults to 10. Lots of servers will ignore this.| |paged |enable and/or configure automatic result paging| -Responses from the `search` method are an `EventEmitter` where you will get a -notification for each `searchEntry` that comes back from the server. You will -additionally be able to listen for a `searchReference`, `error` and `end` event. -Note that the `error` event will only be for client/TCP errors, not LDAP error -codes like the other APIs. You'll want to check the LDAP status code -(likely for `0`) on the `end` event to assert success. LDAP search results -can give you a lot of status codes, such as time or size exceeded, busy, -inappropriate matching, etc., which is why this method doesn't try to wrap up -the code matching. +Responses inside callback of the `search` method are an `EventEmitter` where you will get a notification for +each `searchEntry` that comes back from the server. You will additionally be able to listen for a `searchRequest` +, `searchReference`, `error` and `end` event. +`searchRequest` is emitted immediately after every `SearchRequest` is sent with a `SearchRequest` param. You can do op +like `client.abandon` with `searchRequest.messageID` to abandon this search request. Note that the `error` event will +only be for client/TCP errors, not LDAP error codes like the other APIs. You'll want to check the LDAP status code +(likely for `0`) on the `end` event to assert success. LDAP search results can give you a lot of status codes, such as +time or size exceeded, busy, inappropriate matching, etc., which is why this method doesn't try to wrap up the code +matching. Example: @@ -298,6 +298,9 @@ const opts = { client.search('o=example', opts, (err, res) => { assert.ifError(err); + res.on('searchRequest', (searchRequest) => { + console.log('searchRequest: ', searchRequest.messageID); + }); res.on('searchEntry', (entry) => { console.log('entry: ' + JSON.stringify(entry.object)); }); diff --git a/lib/client/client.js b/lib/client/client.js index bdff844..b34aa8e 100644 --- a/lib/client/client.js +++ b/lib/client/client.js @@ -1251,7 +1251,9 @@ Client.prototype._sendSocket = function _sendSocket (message, conn.end() } else if (emitter) { sentEmitter = true - return callback(null, emitter) + callback(null, emitter) + emitter.emit('searchRequest', message) + return } return false } diff --git a/lib/client/search_pager.js b/lib/client/search_pager.js index d44f318..cafc8e8 100644 --- a/lib/client/search_pager.js +++ b/lib/client/search_pager.js @@ -55,6 +55,7 @@ function SearchPager (opts) { this.started = false const emitter = new EventEmitter() + emitter.on('searchRequest', this.emit.bind(this, 'searchRequest')) emitter.on('searchEntry', this.emit.bind(this, 'searchEntry')) emitter.on('end', this._onEnd.bind(this)) emitter.on('error', this._onError.bind(this)) diff --git a/test/client.test.js b/test/client.test.js index 8fe6194..d3ea243 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -777,14 +777,23 @@ tap.test('search paged', { timeout: 10000 }, function (t) { t.test('paged - no pauses', function (t2) { let countEntries = 0 let countPages = 0 + let currentSearchRequest = null t.context.client.search('cn=paged', { paged: { pageSize: 100 } }, function (err, res) { t2.error(err) res.on('searchEntry', entryListener) + res.on('searchRequest', (searchRequest) => { + t2.ok(searchRequest instanceof ldap.SearchRequest) + if (currentSearchRequest === null) { + t2.equal(countPages, 0) + } + currentSearchRequest = searchRequest + }) res.on('page', pageListener) res.on('error', (err) => t2.error(err)) - res.on('end', function () { + res.on('end', function (result) { t2.equal(countEntries, 1000) t2.equal(countPages, 10) + t2.equal(result.messageID, currentSearchRequest.messageID) t2.end() }) @@ -797,8 +806,11 @@ tap.test('search paged', { timeout: 10000 }, function (t) { countEntries += 1 } - function pageListener () { + function pageListener(result) { countPages += 1 + if (countPages < 10) { + t2.equal(result.messageID, currentSearchRequest.messageID) + } } }) }) From d6ffd550e93fb9889d0178aeece2d10a9355f357 Mon Sep 17 00:00:00 2001 From: link Date: Mon, 24 May 2021 13:33:09 +0800 Subject: [PATCH 4/5] Update docs/client.md Co-authored-by: Tony Brix --- docs/client.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/client.md b/docs/client.md index 60666f9..7ff9f37 100644 --- a/docs/client.md +++ b/docs/client.md @@ -279,7 +279,7 @@ containing the following fields: Responses inside callback of the `search` method are an `EventEmitter` where you will get a notification for each `searchEntry` that comes back from the server. You will additionally be able to listen for a `searchRequest` , `searchReference`, `error` and `end` event. -`searchRequest` is emitted immediately after every `SearchRequest` is sent with a `SearchRequest` param. You can do op +`searchRequest` is emitted immediately after every `SearchRequest` is sent with a `SearchRequest` parameter. You can do operations like `client.abandon` with `searchRequest.messageID` to abandon this search request. Note that the `error` event will only be for client/TCP errors, not LDAP error codes like the other APIs. You'll want to check the LDAP status code (likely for `0`) on the `end` event to assert success. LDAP search results can give you a lot of status codes, such as From fb6d18a48dcdda985f59a515e5af0cc1b5942cf3 Mon Sep 17 00:00:00 2001 From: zhaoyunfeng Date: Mon, 24 May 2021 13:33:19 +0800 Subject: [PATCH 5/5] lint: fix lint error. --- test/client.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/client.test.js b/test/client.test.js index d3ea243..d661bbb 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -806,7 +806,7 @@ tap.test('search paged', { timeout: 10000 }, function (t) { countEntries += 1 } - function pageListener(result) { + function pageListener (result) { countPages += 1 if (countPages < 10) { t2.equal(result.messageID, currentSearchRequest.messageID)