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?

[manast]

psubscribe/subscribe is a redis command which requires a connection to redis. Unfortunately for consistent initialisation of the Queue we cannot rely on ioredis offline command queue.
If you want to contribute with an optimization that could be to call isRedisReady only once and the store the promise and use that promise everywhere instead of calling isRedisReady.

[niftylettuce]

This isn't initialization of the queue though, this is just event listeners being bound? I would submit a contribution, but I don't want to pollute global scope, and it seems like this is unnecessary to begin with - psubscribe/subscribe does not require a connection established yet afaik.

[manast]

subscribe commands are redis commands and therefore require a connection. Feel free to improve the code with a PR, there is not a lot of value in continuing discussing this issue back and forth.

[niftylettuce]

@manast can you make a PR and publish a new version with this? I would gladly sponsor your efforts on GitHub and/or send you a PayPal tip!