Handle connection refused and connection timeouts gracefully

[TPXP]

This pull request rebases the work from #433 to the next branch. It also adds tests to the two cases concerned by the PR:

• connection refused (LDAP server not started)
• connection timeout (firewall problem)

I took this as a opportunity to dive deeper into the code to understand what was actually causing the error to be thrown, I hope the code is a little cleaner than a blind handling of all errors (which caused other tests to fail). The issue was a poor handling of connection errors in the bind method. Since it's the first async method, it is the one that should report connection issues to the server, which it did not. Another issue was the handling of connection refused errors that caused a general error in ldapjs. Sending the exception to the bind callback avoids crashing the whole node process.

[UziTech]

The issue was a poor handling of connection errors in the bind method. Since it's the first async method, it is the one that should report connection issues to the server

Technically ldapjs.createClient is an asynchronous function, it just uses an event emitter instead of a callback.

I'm curious why this wouldn't work to catch the errors:

const client = ldapjs.createClient(...)
client.on('error', (err) => {
	if (err.code === 'ECONNREFUSED') {
		// handle connection refused (LDAP server not started)
	}
}).on('connectError', (err) => {
	// handle connection timeout (firewall problem)
})
client.bind(...)

Niether of these are bind errors so it seems weird to handle them in the bind callback.

[UziTech]

since this only emits errors in connect should this just be changed to emit 'connectError'?

Note: I believe this could be a breaking change.

[TPXP]

Those errors already emit a connectError event on

node-ldapjs/lib/client/client.js

Line 803 in 5afa677

self.emit('connectError', err)

This will emit another (more specific) error event.

On one hand, this can be a breaking change for codes handling connection errors with the error event. On the other hand, the callback for bind will be called with the error so previously written code will still work (it will just call another error handling code).

[jsumners]

I think I am okay with the breaking change here since it will be going into a new major version. I have stated that the intention is to not drastically alter the API, but some breaking changes are going to make it in.

[TPXP]

@UziTech Thanks for the prompt feedback! <3

Granted, binding to the error event will work for handling connection refused errors, while binding to the connectError will handle connection timeouts. That's two events to bind to to make the code resilient and not crash (connection refused) or do nothing (timeout) if the server is not available (uncaught error events crash the node process, which is the motivation of #433). This requires advanced knowledge of the library events (undocumented), and I don't think users wanting a quick start will go through all this for handling these basic events.

I don't find it particularly shocking to emit connection errors while logging into the server, for example that's what the mysql module will do.

[UziTech]

That's two events to bind to to make the code resilient and not crash

That is why I think we should make the connection refused error emit a 'connectError' instead of an 'error' so you will only need to handle one event.

There are also events for 'socketTimeout', 'setupError', 'connectTimeout', and 'timeout'. Maybe we need to consolidate them to make them not so confusing. And probably document them better.

for example that's what the mysql module will do.

I do think we should move the connect() call out of the constructor so this might be less confusing.

the code would have to change to:

const client = ldapjs.createClient(...)
client.connect((err) => {
	// handle connection errors
	client.bind(...)
})

[jsumners]

Next release will be a semver major. So some breaking changes are acceptable. I would like to keep them to a minimum, however.

The connect() inside the constructor is quite annoying --

node-ldapjs/test/client.test.js

Lines 371 to 387 in 5a31aba

	// TODO: this test is really flaky. It would be better if we could validate
	// the options _withouth_ having to connect to a server.
	// t.test('attaches a child function to logger', async t => {
	// /* eslint-disable-next-line */
	// let client
	// const logger = Object.create(require('abstract-logging'))
	// const socketPath = getSock()
	// const server = ldap.createServer()
	// server.listen(socketPath, () => {})
	// t.tearDown(() => {
	// client.unbind(() => server.close())
	// })
	// client = ldap.createClient({ socketPath, log: logger })
	// t.ok(logger.child)
	// t.true(typeof client.log.child === 'function')
	// })

But that might be a bit drastic of a change. A connect must be done before a bind can be performed. So it would introduce a complexity i bootstrapping a client. I can be convinced to make the change, but will need to be sufficiently documented.

Also, we definitely need documentation for the supported events -- #340

[TPXP]

That is why I think we should make the connection refused error emit a 'connectError' instead of an 'error' so you will only need to handle one event.

This current code already does this, it's just that it will emit another crash-causing error event.

Perhaps an acceptable compromise would be to move the connect method call in the bind if not yet called, so that it makes more sense to throw errors from there, but purists would still be able to catch connection errors by calling connect beforehand? This way, previously written code would still work

[UziTech]

Perhaps an acceptable compromise would be to move the connect method call in the bind if not yet called

Why not go further and include the connect and bind call in the search if not yet called?

const client = ldapjs.createClient({username, password, ...});
client.search(...);

[UziTech]

I'm thinking we should leave ldapjs as a low level library and not try to do too much.

What do you guys think about abolishing callback errors and just using event emitter for errors? I can't think of another library that uses event emitter and callbacks for errors.

It seems like the confusion comes from having the event emitter and callbacks.

Looking through the code I have found a few other places where it is confusing whether the error should be sent to the callback or the event emitter or both.

node-ldapjs/lib/client/client.js

Lines 933 to 936 in ad451ed

	self.emit('error', err);
	sock.destroy();
	});
	callback(null);

[jsumners]

I think connection level events make sense being reported through the event emitter interface. Same with streaming parts of the API. But async methods that don’t deal with events should return callback style errors.

I think most of this will shake out as we restructure the code into more maintainable chunks (files).

[jsumners]

@TPXP do you have any thoughts on @UziTech's comments?

[TPXP]

Hello there,

I agree that using callbacks and events for error handling is counter-intuitive, however I'd favor callback-style error handling for async methods as @jsumners suggested since it should make it easier to react to them in accordance with the context in which LDAPjs was called. For example, when building a web app, one would not want to avoid creating a client at every request and bind to events manually. If the callback gives an error object if something goes wrong, it would enable users to directly answer to this request

const app = require('express')()

const ldap = ldapjs.createClient(...)
ldap.bind('dn=directory', ...)

app.get('/search', (req, res) => {
  ldap.search(req.body.query, ..., ..., (data, err) => {
    // I want any errors that occured during the search here (be it a bad request or a timeout, or whatever)
    if(err) res.end('oops, something went wrong')
    else res.json(data).end()
  }
}

Furthermore, if I were to bind to events in my request, I may catch events related to another request, which would be confusing. Or I would have to create a client at every request which would cause performance issues (lots of sockets).

TL;DR: Callback-style errors, please (which is actually sort of what this PR does for connection error events while bind-ing).

I don't think it's a good idea to call the connect method if it was not called previously for other methods than bind. Indeed, bind-ing is usually a compulsory step when working with a remote server so the fact that connect is only called here if needed is acceptable. It would "force" developers to confirm that they want to work anonymously since they would have to call connect manually. As said in the docs, anonymous operations are usually not what you want:

So, lesson #1 about LDAP: unlike HTTP, it's connection-oriented; that means that you authenticate (in LDAP nomenclature this is called a bind), and all subsequent operations operate at the level of priviledge you established during a bind. You can bind any number of times on a single connection and change that identity. Technically, it's optional, and you can support anonymous operations from clients, but (1) you probably don't want that, and (2) most LDAP clients will initiate a bind anyway (OpenLDAP will), so let's add it in and get it out of our way.

So as long as it's said next to the bind method documentation that for connecting anonymously all you have to do is call connect instead of bind, it should be fine.

[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.