ldapjs / node-ldapjs

LDAP Client and Server API for node.js
http://ldapjs.org
MIT License
1.61k stars 448 forks source link

Client.search overwrites earlier searches before completion #278

Closed jvanalst closed 9 years ago

jvanalst commented 9 years ago

So I discovered that when I call LDAP search, if I do two searches in parallel (saying using Bluebird Promises), the first search never emits any events. There are no searchEntrys, no errors, and no end. It appears that even though API leaves the impression that you can perform two searches across the same client in parallel, you really can't the second search actually overwrites the first search, causing the first search to hang forever.

var ldap = require('ldap');
var Promise = require('bluebird');

var ims = ldap.createClient({
  url: 'ldaps://directory'
});

var imsSearch = Promise.promisify(ims.search, ims);
var imsSearchComplete = function (dn, opts) {
  return imsSearch(dn, opts).then(function (res) {
    return new Promise(function (resolve, reject) {
      var results = [];

      res.on('searchEntry', function (entry) {
        results.push(entry.object);
      });
      res.on('error', function (err) {
        if (err) throw err;
      });
      res.on('end', function () {
        resolve(results);
      });
    })
  });
};

Promise.props({
  classes: imsSearchComplete(
    'ou=calendar',
    {
      scope: 'sub',
      filter: '(&(id=667)(objectClass=OrgClass))'
    }
  ),
  persons: imsSearchComplete(
    'ou=people',
    {
      scope: 'sub',
      filter: '(objectClass=OrgPerson)'
    }
  )
}).then(function (results) {
  //this never fires because the results.classes promise never resolves because the 'end' event is never emitted. Only results.persons resolves correctly.
});
jvanalst commented 9 years ago

I.e. Even though the LDAP client API appears to imply making multiple calls at once to the same client is possible, in actuality you have to wait for each search to end before starting the next search.

pfmooney commented 9 years ago

It would be valuable to enable debug logging in ldapjs (via node-bunyan) to ensure that this issue is not caused by external control flow problems.

jvanalst commented 9 years ago

I went to test this again to see if I could figure out the bunyan logger stuff and found the the order of the promises even affects whether they successfully finish or not.

If the persons search is second the classes search never finishes. But if classes is second they both finish successfully. Weird.

The persons search is expected to return oodles of records, the classes only 1.

jvanalst commented 9 years ago

I have a log but I have to figure out how to redact it and still keep it useful for you, because it's full of personal information I can't expose (it's also 300 lines long because of the number of persons retrieved from LDAP).

I can see both searches in the log. I can also see SearchEntries for the one that never emits events, as well, so it does ultimately get and parse that result from the server. Again the callbacks 'res' argument never triggers the appropriate events for some reason.

jvanalst commented 9 years ago

After analyzing the logs myself it did end up being the external flow control. Something about running promisify causes the callback registering the event listeners to be delayed until after those events had already fired in some instances.

When I rewrote it as a flat promise it worked as expected.

javefang commented 8 years ago

Hi @jvanalst, I'm having exactly the same problem as you. I tried to write a flat promise as you have suggested but no luck. It works fine when I'm using "ldap", but not "ldaps". I saw you are also using ldaps, could you provide some details on how you solved the problem, please? Thanks.

'use strict';

var ldap = require('ldapjs');
var _ = require('lodash');
var bluebird = require('bluebird');

var LDAP_ADDR = 'ldaps://ldapserver';
var USER = 'user';
var PASSWORD = 'password';
var CAS = ['OUR_CA']

var log = console.log;

var users = ['user_a', 'user_b', 'user_c'];

var client = ldap.createClient({
  url: LDAP_ADDR,
  tlsOptions: {
    ca: CAS
  }
});

var base = 'OU=Users,DC=com';
var userCount = users.length;
var searchCount = 0;
var endCount = 0;

function searchAsync(user) {
  return new Promise(function (resolve, reject) {
    var opts = {
      filter: '(samAccountName=' + user + ')',
      scope: 'sub',
      attributes: ['dn']
    };

    client.search(base, opts, function(err, result) {
      var entries = [];

      var currentCount = ++searchCount;
      log('SEARCH_CB: %s/%s', currentCount, userCount);

      result.on('searchEntry', function (entry) {
        // log(entry.object.dn);
        entries.push(entry);
      });

      result.on('error', function (err) {
        log('ERROR');
        reject(err);
      });

      result.on('end', function (result) {
        endCount++;
        log('END: %s/%s', currentCount, userCount);
        resolve(entries);
      });
    });
  });
}

// callback style
client.bind(USER, PASSWORD, function () {
  return bluebird.map(users, searchAsync)
    .then(function () {
      log('unbind');
      client.unbind();
    });
});
jvanalst commented 8 years ago

I create and end a new client as part of every search (promise). It's a bunch of overhead, but it seemed unavoidable.

javefang commented 8 years ago

@jvanalst I'm trying a possible solution which might work. https://github.com/mcavage/node-ldapjs/issues/329

jsumners commented 1 year ago

⚠️ This issue has been locked due to age. If you have encountered a recent problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example when opening a new issue.