Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Concurrent calls to client.search() failed when using "ldaps" #329

Closed
javefang opened this issue Dec 16, 2015 · 6 comments
Closed

Concurrent calls to client.search() failed when using "ldaps" #329

javefang opened this issue Dec 16, 2015 · 6 comments
Labels

Comments

@javefang
Copy link

I'm wondering if the functions on a client can be called concurrently?

I have a small test application which takes a list of usernames, fetch their details from ldap using ldapjs and print them to the console. Currently I implement it in the way that it create a single ldapjs client, bind it and then call "search" for each of the usernames concurrently.

It works fine when I'm connecting with unsecured "ldap" protocol. But when switched to "ldaps", all search still returns the event emitter, but some of them will never call "end" or "error".

I'm wondering if it is safe to call another search while one is still not finished? Thanks.

Environment:
windows 7
node v0.12.9
ldapjs 1.0.0

@javefang
Copy link
Author

might have found the problem:

in lib/client/client.js, on the last few lines of function "Client.prototype._sendSocket",

conn.write(message.toBer(), writeCallback);

If I understand correctly, this guarantees that the message (search request) is written to the socket before calling writeCallback, which will return the event emitter back to the caller. However, this might be too late in some cases. As I have noticed that the "end" event is fired before the callback of "search" is called. In which case the caller will never be able to receive that event as it hasn't got a chance to register event handler yet.

Maybe we should make sure the emitter is returned to the caller so that they have the chance to register event handler before any event arrive? My initial thought is as follow. Hopefully you could provide more help on this?

try {
    writeCallback();
    return conn.write(message.toBer());

  } catch (e) {
    if (timer)
      clearTimeout(timer);

    log.trace({err: e}, 'Error writing message to socket');
    emitter.emit('error', e);
  }

@javefang
Copy link
Author

btw, the issue I'm having only seem to affect "ldaps" protocol so far. and the above fix works for me.

@pfmooney
Copy link
Contributor

@javefang Could you collect trace-level logs from this situation?

rdubigny pushed a commit to rdubigny/node-ldapjs that referenced this issue Feb 1, 2018
As per issue ldapjs#329 a race condition can occur that will emit events before the callback has had a chance to listen to them. We were able to replicate this about 60% of the time, causing timeout issues internally when in fact while looking at the trace logs, the search results returned.

Easiest to replicate with single result searches.

With these changes, we 100% verified that we no longer are missing events and it works as intended.

Thanks to @javefang for the fix!
tofixx pushed a commit to hpi-schul-cloud/node-ldapjs that referenced this issue Apr 23, 2019
As per issue ldapjs#329 a race condition can occur that will emit events before the callback has had a chance to listen to them. We were able to replicate this about 60% of the time, causing timeout issues internally when in fact while looking at the trace logs, the search results returned.

Easiest to replicate with single result searches.

With these changes, we 100% verified that we no longer are missing events and it works as intended.

Thanks to @javefang for the fix!
@Loki-Afro
Copy link

any plan in integrating this? there is also a fork with this fix only based on an old version .. hpi-schul-cloud@3d470ab

@UziTech
Copy link
Member

UziTech commented Jul 14, 2021

If someone wants to create a PR with the fix we can look at it. Make sure to add a test as well.

@jsumners
Copy link
Member

👋

On February 22, 2023, we released version 3 of this library. As a result, we are closing this issue/pull request.

Please see issue #839 for more information, including how to proceed if you feel this closure is in error.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Feb 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants