Skip to content

Add possibility to configure socket timeout #1358

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

Open
uxon123 opened this issue Jun 26, 2018 · 7 comments
Open

Add possibility to configure socket timeout #1358

uxon123 opened this issue Jun 26, 2018 · 7 comments
Labels

Comments

@uxon123
Copy link

uxon123 commented Jun 26, 2018

Thanks for wanting to report an issue you've found in node_redis. Please delete
this text and fill in the template below. Please note that the issue tracker is only
for bug reports or feature requests. If you have a question, please ask that on gitter.
If unsure about something, just do as best as you're able.

Note that it will be much easier to fix the issue if a test case that reproduces
the problem is provided. It is of course not always possible to reduce your code
to a small test case, but it's highly appreciated to have as much data as possible.
Thank you!

  • Version: 2.8.0
  • Platform: Node.js 8.11.1on node:8-alpine docker image on Kubernetes 1.9.3
  • Description: On different environments different default tcp timeouts might occur. Currently AFAIK node-redis doesn't allow to configure socket timeout. I find it problematic on my prod environment, when redis server (behind a k8s service, a kind of load balancer) is down for couple of seconds, node_redis client is hanged trying to reconnect. Even though redis starts responding, the client is still hanged until it reaches a timeout which seems to be 130 seconds in my case.
    I would like to configure redis client to wait only a given timeout.
    The deprecated param connect_timeout seems to not work as expected.
    Is there any way to configure socket timeout in node-redis?
@stockholmux
Copy link
Contributor

If you create your own net instance with RedisClient's second argument you can put anything you'd like in as far as options.

Be warned that it's not an official feature, and it may go away.

@jordie23
Copy link

Oops hit delete on my comment.

I'm having this exact issue.

It seems like connect_timeout, despite being marked as 'deprecated' in the documentation, is actually used for the net instance regardless of whether you have a retry_strategy defined. Looks like it's used for two things? The net instance timeout and the legacy retry strategy. Perhaps it should be split into two? Add a new option specifically for the net instance?

This looks like where it is https://github.com/NodeRedis/node_redis/blob/04a09aa2ffcbae9f380b61a4afe15badfe5be5ef/index.js#L255

Correct me if I'm wrong about any of this please, I'm trying to tackle this issue as well.

@billinghamj
Copy link

This really needs to be addressed ASAP. Right now, if your Redis server ever becomes unavailable for more than a few seconds, the library becomes unsafe/unusable.

If you set the connect_timeout to a low value, it will give up and stop bothering to try and reconnect very quickly.

If you set it to a high value (or leave it as the default), it takes 3+ mins to recover and reconnect - no matter what other options you use.

This has been brought up before (#652), but the PR was closed, seemingly without understanding that the implemented solution did not resolve the issue. At present, other than using the undocumented second argument, there is simply no way to use this library safely.

@billinghamj
Copy link

billinghamj commented Jul 25, 2018

Alternatively, as it does seem fairly unlikely that this package will ever be updated again, this library is mostly compatible and has a much more sane approach to timeouts/retrying/etc.:

https://github.com/luin/ioredis

Looks like a new major version of that lib (v4) is currently in beta, so might be worth waiting until it is released.

@stockholmux
Copy link
Contributor

@jordie23 and @billinghamj I would entertain a PR that resolves the issue.

@BridgeAR
Copy link
Contributor

BridgeAR commented May 7, 2019

This seems to be related to #1430

@Salakar Salakar modified the milestones: v3.0.0, v4.0.0 Feb 7, 2020
@keith-chew
Copy link

Hi

I wrote a comment in:

#1488 (comment)

On how we can recreate the redis client from the app level once the client has ended (eg from connect_timeout), both in a blocking and non-blocking way. But yes, ideally, it should be handled by the underlying library.

@leibale leibale removed this from the v4.0.0 milestone Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants