redis
Add a way for configurable socket timeouts
Pull Request check-list
Please make sure to review and check all of these items:
- [x] Does
npm testpass with this change (including linting)? - [x] Is the new or changed code fully tested?
- [x] 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 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.
@BridgeAR I would support putting this in because you would need to access the the stream property directly on a
connectevent 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 LGTM. Please resolve the conflicts with master, and I'll merge it.
@leibale updated the branch, there are some failures but do not seem to be related to my changes.
@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
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.
@dmaier-redislabs Any chance we can get this (or something similar) merged soon?
@bobymicroby @elena-kolevska Can you please take a look and see if this can be merged before the next release?
@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 probably, but cannot make commitments ; will ping you if there is movement on that
