Add a way for configurable socket timeouts

[flippmoke]

Pull Request check-list

Please make sure to review and check all of these items:

• Does npm test pass with this change (including linting)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Added a way for connected sockets to timeout. This is important for situations where a connected server stops responding and the connection should close in a reasonable time.

[BridgeAR]

The code itself is LGTM. I have to give this another thought if it's best to better document to access the stream property directly as user to do these things or to add "native" option instead.

I would be happy about feedback from others about this!

[flippmoke]

@BridgeAR I would support putting this in because you would need to access the the stream property directly on a connect event and this seems like a lot of extra work. Additionally, the other timeout option is confusing because its easy to assume that this timeout does exactly what this timeout option specifies.

[DanielSeehausen]

@BridgeAR I would support putting this in because you would need to access the the stream property directly on a connect event and this seems like a lot of extra work. Additionally, the other timeout option is confusing because its easy to assume that this timeout does exactly what this timeout option specifies.

👍 for @flippmoke comment @BridgeAR

[flippmoke]

Bump on this?

[leibale]

@flippmoke LGTM. Please resolve the conflicts with master, and I'll merge it.

[flippmoke]

@leibale updated the branch, there are some failures but do not seem to be related to my changes.

[leibale]

@flippmoke please fix those eslint errors:
line 377 in test/connection.spec.js - Trailing spaces not allowed
line 363 in index.js - Missing space before function parentheses

[flippmoke]

@leibale sorry I missed those pushed now

[orzelek12]

Hi @leibale @flippmoke @BridgeAR
can you merge that PR and release a new version?
as u can see in Issues section there are a lot of ppl that find the lack of this feature disturbing.
In case of Redis server being unresponsive, the current implementation node-redis will cause the app to break since it will try to connect over an over again.
Please merge this changes in order to allow us stop the connection after given time.

[philon-msft]

@dmaier-redislabs Any chance we can get this (or something similar) merged soon?

[dmaier-redislabs]

@bobymicroby @elena-kolevska Can you please take a look and see if this can be merged before the next release?

[bobymicroby]

@philon-msft The code base has evolved significantly during the last few years, so this PR will not be easily mergeable. We recognize the need for such functionality (we have added this feature to ioredis recently), but we also recognize that such features need to be implemented carefully (see here a subsequent fix for connection instability caused by the socket timeout feature).

We are going to add this feature to the client with a new PR.

[philon-msft]

Thanks @bobymicroby, will the new PR make it into the next release?

[bobymicroby]

@philon-msft probably, but cannot make commitments ; will ping you if there is movement on that

[nkaradzhov]

Hi, this is the new PR for this change