Node-ldapjs: Search entries emitted too soon

[ifroz]

When using the client.search() API, I encountered an issue, where my client sometimes missed the first few searchEntry events (or even the end event).
Apparently, if the LDAP server is much faster than the client, the client does not have enough time to bind event listeners in the callback.

In order to mitigate this issue, I published the fork ldapjs-with-injectable-emitter,
which, in it's search options, accepts an optional EventEmitter instance, so that the user can set up all bindings before actually firing off the request. It can be used as follows:

return new Promise((resolve, reject) => {
  const entries = []

  const emitter = new EventEmitter()
  emitter.on('searchEntry', entry => { users.push(entry) })
  emitter.on('error', err => reject(err))
  emitter.on('end', () => resolve(entries))

  client.search('ou=People', { emitter }, (err, res) => {
    if (err) reject(err)

    // Calling res.on(...) here might be too late
    res === emitter // => true
  })
})

This fixes my use-case, but the issue still remains for users of v1.0.2.
Therefore I'd like to open discussion regarding to either implement this approach in ldapjs or come up with another solution, so that ldapjs users can mitigate this caveat.

[jsumners]

Is this an issue in ldapjs@next? Can you replicate it in the unit/integration tests?

[ifroz]

• I experience the same issue using v2.0.0-pre.5 (and v1.0.2)
• I have partial success reproducing the issue in unit tests, but this setTimeout-based hanging test aims to prove my point.
• In my use-case, I use node v10.15.0 (I see that some tests on master hang in node >= 10)

[jsumners]

New work should be developed against the next branch.

[ifroz]

Same behaviour reproduced towards the next codebase: next...ifroz:next (at 11868021918)

[rkaw92]

Hi, I've just authored a patch (linked above) against next that addresses this without any API changes, by using a custom EventEmitter that stores unemitted events and only emits them when a listener is added. This is necessary because, while it seems like the original API was meant to emulate that of Node.js Streams, it failed to do so reliably because it lacked a "cork" / "flowing mode" distinction that actual Streams have.

Streams do internal buffering and only start emitting entries when a listener for data is added - this is properly called switching into "flowing mode", where new data is emitted as it arrives. This is what we eventually want, but only after ensuring that the programmer has had a chance to register event listeners.

Thanks to the test by @ifroz, I had good assertions ready to verify against.

Please let me know what you think. We'll also get around soon to testing this manually on a specific host where an issue with missing search results (supposedly this) is known to occur predictably.

Also, I've had some issues running the tests locally where 2 tests would fail on next right after cloning from the upstream and doing an npm install, on Node 12.16.1 - my patch doesn't seem to introduce any additional failures, however, and running the newly-added tests in isolation with --only yields success. Any assistance here would be appreciated - shall I paste the test failures I'm having, or can somebody else try the upstream next branch and check if they concur?

[UziTech]

Great work @rkaw92! If you could create a PR against the next branch with that commit the GitHub CI could run the tests and we could see any issues there.

[rkaw92]

Update:

• Created a PR, as seen above. Of course the tests pass now in the test environment! :confused: The failures are still reproducible on my machine, but they seem unrelated to the problem or the fix. Pushing these out of scope for now.
• We've found out that the reason for the missing entries on the other machine was, in fact, not this bug. Another bug report + fix can be expected soon...

[jsumners]

@rkaw92 I look forward to your next fix. As for the test failing issue, if you can open a new issue with a paste of you test output I'll see if anything jumps out at me.

Closing this issue as solved by #610.