Adding patch for ldaps concurrent failures.

[renevo]

As per issue #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!

[javefang]

Glad that you got this to work! Nice job, @pfmooney any chance you could take a look at this PR, please?

[javefang]

Any chance we can merge this PR? Without it the ldaps:// protocol is not really usable.

[cvillemure]

I have the same issue with LDAP/TLS where about 1% of the search never receive the "end" event. This commit seems to fix the issue.

[rdubigny]

Any chance to get this merged?

It worked like a charm on our project witch makes hundreds of concurrent call to our LDAP server.

As this project does not seems to be maintained any more, we will start using your fork from now.

[rdubigny]

@renevo can you publish your fork on npm?

[renevo]

Since this is "technically" managed by Joyent now, would want to find out if they are going to merge this or not first.

If not, will find out if we can.

[renevo]

@melloc thoughts?

[rdubigny]

@renevo I will publish a fork from your repo since we need this on our project quite soon. I'll be glad to fetch your own publication if you publish it.

According to #461 , I don't expect an update on this repo.

[rdubigny]

@renevo I just published the ldapjs-concurrency-fix package.

[melloc]

@renevo Sorry for the delayed reply. I haven't had a chance since migrating this and several other packages over to go through all the pending merge requests and bugs, but I'd like to try to do so next week. Was there a reason you moved the writeCallback away from the .write() call? At first glance, I would expect it to not be called until the write succeeds.

[renevo]

The emitter was not able to be wired up properly due to not having been sent to the callback yet.

#329 explains this pretty well, the fix was to call the callback so the emitter can be listened, then actually send the request across the wire. (writeCallback, then send message)

[renevo]

This can be reproduced (in our case) by having an ldap search happen on a single connection through HTTP API over STARTTLS or TSL LDAP connection.

[renevo]

Any progress on this @melloc ?

[jsumners]

👋 new maintainer here. I want to merge this but I need to understand it better first. Some concerns:

1. The original implementation is passing the write callback as the callback for the write (see: https://nodejs.org/dist/latest-v10.x/docs/api/net.html#net_socket_write_data_encoding_callback). It seems out of order to invoke the callback before the operation has completed. Is there not a more appropriate way to accomplish this goal?

2. Would you be able to add a unit test for this functionality?

[jsumners]

⚠️ 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.