Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 task done
edvinv opened this issue Jan 17, 2024 · 6 comments · Fixed by #2425
Closed
1 task done

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

edvinv opened this issue Jan 17, 2024 · 6 comments · Fixed by #2425
Labels
bug Something isn't working

Comments

@edvinv
Copy link

edvinv commented Jan 17, 2024

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
@edvinv edvinv added the bug Something isn't working label Jan 17, 2024
@arunp0
Copy link

arunp0 commented Jan 29, 2024

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
Copy link
Author

edvinv commented Feb 5, 2024

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.

github-actions bot pushed a commit that referenced this issue Feb 16, 2024
## [5.1.12](v5.1.11...v5.1.12) (2024-02-16)

### Bug Fixes

* **redis-connection:** close redis connection even when initializing ([#2425](#2425)) fixes [#2385](#2385) ([1bc26a6](1bc26a6))
@edvinv
Copy link
Author

edvinv commented Feb 16, 2024

@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
Copy link
Contributor

manast commented Feb 19, 2024

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
Copy link

kibertoad commented Mar 14, 2024

@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
Copy link
Contributor

manast commented Mar 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants