Queue close does not close redis connection when using custom createClient function

[dobesv]

Description

When creating queues using the createClient function to connect to redis, the queues are not closed when the queue is closed.

Minimal, Working Test code to reproduce the issue.

const Queue = require('bull');
const Redis = require('ioredis');
const Bluebird = require('bluebird');

const main = async () => {
  const clients = [];
  const createClient = (type, redisOpts) => {
    const redisOptions = {
      ...redisOpts,
      connectionName: ['bull', type].join('-'),
    };
    const client = new Redis(redisOptions);
    clients.push(client);
    console.log('start redis', redisOptions.connectionName);
    client.on('connect', () =>
      console.log('redis connected', redisOptions.connectionName),
    );
    client.on('end', () =>
      console.log('end redis', redisOptions.connectionName),
    );
    return client;
  };
  const q1 = new Queue('1', { createClient });
  const q2 = new Queue('2', { createClient });
  const q3 = new Queue('3', { createClient });
  q1.process(1, job => console.log('q1', job.data)).catch(err =>
    console.error(err),
  );
  q2.process(2, job => console.log('q2', job.data)).catch(err =>
    console.error(err),
  );
  q3.process(3, job => console.log('q3', job.data)).catch(err =>
    console.error(err),
  );
  console.log('processors ready');
  await Bluebird.delay(1000);
  await Promise.all([q1.add({ q: 1 }), q2.add({ q: 2 }), q3.add({ q: 3 })]);
  await Bluebird.delay(1000);
  await q1.close();
  console.log('closed q1');
  await q2.close();
  console.log('closed q2');
  await q3.close();
  console.log('closed q3');
  Object.values(clients).forEach(client => {
    console.log(client.options.connectionName, client.status);
  });
  console.log('end of main');
};

main();

Output:

$ NODE_OPTIONS=--max-old-space-size=100 node ./reproduce2.js 
start redis bull-client
start redis bull-client
start redis bull-client
processors ready
redis connected bull-client
redis connected bull-client
redis connected bull-client
start redis bull-subscriber
start redis bull-subscriber
start redis bull-subscriber
redis connected bull-subscriber
redis connected bull-subscriber
redis connected bull-subscriber
start redis bull-bclient
start redis bull-bclient
redis connected bull-bclient
start redis bull-bclient
redis connected bull-bclient
redis connected bull-bclient
q1 { q: 1 }
q2 { q: 2 }
q3 { q: 3 }
end redis bull-bclient
redis connected bull-bclient
closed q1
end redis bull-bclient
redis connected bull-bclient
closed q2
end redis bull-bclient
redis connected bull-bclient
closed q3
bull-client ready
bull-client ready
bull-client ready
bull-subscriber ready
bull-subscriber ready
bull-subscriber ready
bull-bclient ready
bull-bclient ready
bull-bclient ready
end of main
^C

It seems like it may have been intentional to not disconnect these redis clients, based on this little bit of logic:

      if (!options.createClient) {
        queue.clients.push(client);
      }

However, when you look at the output, it actually does close one of the redis clients (bclient) when you close the queue, but immediately reconnects it afterwards.

Bull version

3.14.0

Additional information

See also #1414

[bymartinez]

I have this same issue, any updates with this issue?

[gabegorelick]

It seems like it may have been intentional to not disconnect these redis clients, based on this little bit of logic

I think this is correct. Existing Redis connections may be used by others, so it's not safe for Bull to close them on queue close. Using this behavior to pool connections between Bull and other clients is very common.

However, when you look at the output, it actually does close one of the redis clients (bclient) when you close the queue, but immediately reconnects it afterwards.

The blocking client cannot be used by others, since it blocks. In other words, Bull completely owns it. So it's safe for Bull to close.

So I'm not sure there's much of a bug here.

[dobesv]

Existing Redis connections may be used by others, so it's not safe for Bull to close them on queue close.
The blocking client cannot be used by others, since it blocks. In other words, Bull completely owns it. So it's safe for Bull to close.

Maybe the documentation just needs to be updated to make this clearer.

Maybe with createClient docs it should specifically say that the bclient queue is exclusive to each queue and will be disconnected and reconnected by bull based on the queue operations, but the other clients will not be closed.

[gabegorelick]

It mentions some of this in PATTERNS.md, but it could be clearer:

bclient connections cannot be re-used

I don't think createClient is documented in https://github.com/OptimalBits/bull/blob/develop/REFERENCE.md so there's definitely room for improvement.

[joebowbeer]

The documentation should mention that the connections created by createClient are not managed: the provider is responsible for these.

In practice, that means the provider needs to maintain a collection of clients per queue, and disconnect them after the queue is closed.

[dobesv]

I've created a couple of PRs to the documentation to help people avoid this problem.

[reisandbeans]

@dobesv just to be clear, bull will not close client and subscriber connections, but what about bclients? will they be auto closed?

[dobesv]

bclient connections are closed when the queue is closed, I believe.

[reisandbeans]

@dobesv I was writing some integration tests for my code, and I noticed something. Previously, I was manually storing and closing all connections (client, subscriber and bClients), and my test would execute and finish (I'm using Jest, by the way). Then, after going through this thread, I did some cleanup in my code and stopped storing/closing bclient connections. Even though my tests still pass, jest keeps running after the test as if something is still open/running. I tried to use --detectOpenHandles to check what was left open, but it doesn't give me any details.
Bringing back my old code makes the test exit cleanly.

[dobesv]

@Tinadim Maybe there are still bugs with that approach. I think we stopped trying to supply our own clients and just let bull manage those connections.

[melalj]

@reisandbeans Did you manage to find a solution?