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

Stop Redis connection from preventing shutting down the service #83

Conversation

hectorgomezv
Copy link
Member

Closes #73

This PR aims to prevent Redis connection retries from stopping service shutdown.

I think the main problem was the service used client.quit() to gracefully disconnect from Redis instance, and this forces the app to wait until the Redis instance acknowledges the connection end. The problem arises when the Redis instances is not reachable for any reason (socket issues, the instance is not running, etc.), there is a similar issue open in the node-redis repository.

So the solution I implemented is to define a timeout which will fire a client.disconnect() if the instance doesn't respond in time. I've set a 2 seconds grace period for the server to respond, but we could change this if you think so, or even we could extract this into a configuration param/env variable.

@hectorgomezv hectorgomezv marked this pull request as draft September 8, 2022 15:20
@coveralls
Copy link

coveralls commented Sep 8, 2022

Coverage Status

Coverage decreased (-0.3%) to 88.728% when pulling 968af6e on 73-shutdown-hook-not-working-while-trying-to-connect-to-redis into 905f6fe on main.

@hectorgomezv hectorgomezv self-assigned this Sep 8, 2022
@hectorgomezv hectorgomezv marked this pull request as ready for review September 8, 2022 15:26
*/
private forceQuit() {
this.logger.verbose('Forcing Redis connection close');
this.client.disconnect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disconnect returns a Promise shouldn't we make forceQuit async? How does that play with the bind() in setTimeout?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, maybe it's more correct to define it as async and wait for the Promise to be resolved. Solved here. Thanks.

bind() creates a new function but doing a closure over this (i.e. saving the caller context). Otherwise setTimeout callback (forceQuit function) won't have access to the context when fired. This is necessary to still having access to this.client to invoke disconnect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know about: https://nodejs.org/api/timers.html#timers-promises-api

I did a quick mockup with the following:

import { setTimeout } from 'timers/promises';

...

async onModuleDestroy(): Promise<void> {
  const abortController = new AbortController();
  this.logger.verbose('Closing Redis connection');

  (async () => {
    await setTimeout(this.quitTimeoutInSeconds * 1000, this.forceQuit(), {
      signal: abortController.signal,
    });
  })();

  await this.client.quit();
  abortController.abort();
  this.logger.verbose('Redis connection closed');
}

@hectorgomezv hectorgomezv merged commit 2faa5bd into main Sep 9, 2022
@hectorgomezv hectorgomezv deleted the 73-shutdown-hook-not-working-while-trying-to-connect-to-redis branch September 9, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shutdown hook not working while trying to connect to Redis
3 participants