[Bug]: Redis connection not correctly closed (regression)

[edvinv]

Version

5.1.2

Platform

NodeJS

What happened?

I have updated bullmq from 4.17.0 to 5.1.2 and noticed that connection was not closed correctly when running some tests. When I pin down the problem I found that version 5 is not closing connection if there is no delay between creating and closing.

How to reproduce.

  const queue = new Queue('CALLS_JOB_QUEUE_NAME', {
    connection: {
      host: 'localhost',
      port: 6379,
    },
  });

  await queue.close();
  await promiseDelay(1000);
  console.log("done");

If I run this sample with 5.1.2 I see at console.log("done"), that connection in redis is still opened. Running the same sample with 4.17.0, connection is closed as expected. If I add additional delay before close like this:

  const queue = new Queue('CALLS_JOB_QUEUE_NAME', {
    connection: {
      host: 'localhost',
      port: 6379,
    },
  });

  await promiseDelay(1000); // ADDED!!!
  await queue.close();
  await promiseDelay(1000);
  console.log("done");

Than with both version connection is correctly closed.

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[arunp0]

You are trying to close the connection even before the connection is established.
https://github.com/taskforcesh/bullmq/blob/master/src/classes/redis-connection.ts#L292

When you add delay, it gives enough time for the Redis connection to establish.

Not a bug.

[edvinv]

No, IMHO this is a bug. Connection can be correctly closed even in the case when it was not yet fully established. And it was working like this before commit.

So current code:

        if (status === 'ready') {
          // Not sure if we need to wait for this
          await this.initializing;
          if (!this.shared) {
            await this._client.quit();
          }
        }

should be changed to sommething like:

        if (status === 'initializing' || status === 'ready') {
          // Not sure if we need to wait for this
          await this.initializing;
          if (!this.shared) {
            await this._client.quit();
          }
        }

This is how all libraries works, when you get valid connection handle, this handle can be used to close the connection. It is up to to library to correctly handle the case when connection is not fully established. Imagine a case when you open a connection and then immideately exception is throw, you must be able to close the connection from catch/finnaly so that application can correctly exists.

[edvinv]

@manast , I have tested latest version 5.1.12 and now node process is correctly closed at the end so there are no hanging redis connections left. But I get unhandled promise rejection detected.

Example:

import { Queue } from 'bullmq';

async function test() {
  const callsQueue = new Queue('CALLS_JOB_QUEUE_NAME', {
    connection: {
      host: 'localhost',
      port: 6379,
    },
  });

  await callsQueue.close();
}

process.on('unhandledRejection', error => {
  console.error('Unhandled promise rejection detected.', error);
});
process.on('rejectionHandled', promise => {
  console.error('Promise rejection was handled.');
});

test()
  .then(() => { console.log('Done!'); })
  .catch(err => { console.error(err); });

The output log is:
Done!
Unhandled promise rejection detected. Error: Connection is closed.

[manast]

You will need to attach an error handler to the queue instance to avoid the unhandled exception, like:

callsQueue.on('error', (err) => ... ):

We can agree that the exception should be produced as you are closing a queue that has not yet had a change to connect.

[kibertoad]

@manast Why produce that unhandled rejection, though? It's never going to be useful to the end user, why require everyone to handle it? Closing a connection that wasn't even established ideally should be be a noop.

[manast]

@kibertoad basically because we do not want to have code for specific exceptions that can be generated by ioredis. It is better to let the user handle all exceptions in the same way.