API docs/README should state that a default error event is bound

[niftylettuce]

This wasn't obvious until I got a max listeners exceeded warning.

Ref:

bull/lib/queue.js

Lines 221 to 223 in ce5c91d

	this.on('error', () => {
	// Dummy handler to avoid process to exit with an unhandled exception.
	});

[niftylettuce]

Per: https://github.com/OptimalBits/bull/blob/develop/REFERENCE.md#events

[niftylettuce]

Also, it is not noted anywhere that on and once are overridden with custom logic, which is another cause of this error:

const _on = Queue.prototype.on;

Queue.prototype.on = function(eventName) {
  this._registerEvent(eventName);
  return _on.apply(this, arguments);
};

const _once = Queue.prototype.once;

Queue.prototype.once = function(eventName) {
  this._registerEvent(eventName);
  return _once.apply(this, arguments);
};

via lib/queue.js

[niftylettuce]

I'm encountering these issues in https://github.com/ladjs/bull and https://github.com/ladjs/redis, specifically if you create a new instance of Bull:

const Bull = require('@ladjs/bull');

const bull = new Bull({ ... });

Whereas the options to new Bull include a shared Redis connection.

[manast]

I would accept a PR for this.

[niftylettuce]

Okay will do, I'm still trying to figure out the best approach to preventing duplicate listeners from being added.

[niftylettuce]

@manast I've actually identified a core bug I believe - .isRedisReady(this.eclient) is called here

bull/lib/queue.js

Line 458 in ce5c91d

.isRedisReady(this.eclient)

for every event binded, which in turns adds a .once('error' listener for each event, and if you bind 10 events, then you exceed the default 10 event listener limit.

/**
 * Waits for a redis client to be ready.
 * @param {Redis} redis client
 */
function isRedisReady(client) {
  return new Promise((resolve, reject) => {
    if (client.status === 'ready') {
      resolve();
    } else {
      function handleReady() {
        client.removeListener('error', handleError);
        resolve();
      }

      function handleError(err) {
        client.removeListener('ready', handleReady);
        reject(err);
      }

      client.once('ready', handleReady); <---- here
      client.once('error', handleError); <---- and here
    }
  });
}

[manast]

hmm, I don't see why that code would imply a bug. There is nothing that would suggest that having more than 10 listeners as a bug. This is just a weird warning from Node. It is quite common to setup far more than 10 listeners in browsers for example, and no warnings are emitted...
And as you can see, the listeners are cleaned correctly after the events are emitted, so no memory leaks either.

[niftylettuce]

I think binding these errors should be handled by the user for full control.

[manast]

but how can we implement the isRedisReady then?

[niftylettuce]

I don't think you need to wait for Redis to be ready before psubscribe nor subscribe.

[manast]

If we could get rid of this function I can guarantee we would have done it long time ago...

[niftylettuce]

Binding a ready and error listener for each event listener binded seems like really bad practice to begin with though, would you not agree?

Can you give me context as to why the function is necessary? I searched through ioredis repository and could not find any reason that psubscribe and subscribe would require the connection to be ready.

[manast]

it is a method that waits for the redis instance to be ready, you need to listen to both the ready event and the error event, as soon as it is ready the event listeners are freed. There is nothing wrong with this code!

[niftylettuce]

@manast Why is it required to wait for the Redis instance to be ready* for simply binding subscribe/psubscribe though? You haven't given me a reason. I totally understand event emitters binding once and then being freed. That 100% makes sense. But what does not make sense is why is isRedisReady required to begin with for binding event listeners?