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.

Sign up for GitHub

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

feat: add "timeoutPerCommand" option to detect dead connection #658

Closed
wants to merge 2 commits into from
Closed

feat: add "timeoutPerCommand" option to detect dead connection #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8afd00
feat: add "timeoutPerCommand" option to detect dead connection
luin Jul 6, 2018
d9a9193
Merge branch 'master' into command-timeout
luin Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/redis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ Redis.defaultOptions = {
reconnectOnError: null,
readOnly: false,
stringNumbers: false,
maxRetriesPerRequest: 20
maxRetriesPerRequest: 20,
timeoutPerCommand: null
};

Redis.prototype.resetCommandQueue = function () {
Expand Down Expand Up @@ -612,6 +613,8 @@ Redis.prototype.sendCommand = function (command, stream) {
select: this.condition.select
});

this.lastWriteTime = Date.now()

if (Command.checkFlag('WILL_DISCONNECT', command.name)) {
this.manuallyClosing = true;
}
Expand Down
24 changes: 24 additions & 0 deletions lib/redis/event_handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,25 @@ exports.connectHandler = function (self) {

self.initParser();

self.lastWriteTime = null;
if (typeof self.options.timeoutPerCommand === 'number') {
var timeoutPerCommand = self.options.timeoutPerCommand;
debug('Per-command timeout set to %s', self.options.timeoutPerCommand);
self.timeoutPerCommand = setInterval(() => {
Copy link

Choose a reason for hiding this comment

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

how about using this: https://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback to generate a timeout event from the socket if it becomes inactive while the queue is being processed?

Copy link

Choose a reason for hiding this comment

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

This timeout guards against a socket which has no activity. The value to use probably greatly depends on the use cases. In a production environment, there's probably enough traffic to keep such a timeout from triggering. But in a dev environment, there might not be any traffic for relatively long stretches of time making this an idle connection which then times out. Setting a per command timeout seems better.

Also, if this socket timeout occurs, it doesn't enable specific error handling / recovery for the specific commands which timed out.

Or were you suggesting this as an additional check and not an alternative solution?

Copy link

Choose a reason for hiding this comment

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

I am suggesting to set this to something reasonable while we are processing commands and to disable it while the socket is not used.
It just seems that it can be easier to implement than setting and cancelling timeouts in the lib. And more powerful as well.

var commandQueueLength = self.commandQueue.length;
if (
commandQueueLength > 0 &&
Date.now() - self.lastWriteTime > self.options.timeoutPerCommand
Copy link

Choose a reason for hiding this comment

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

Since self.lastWriteTime is updated every time a command is added to the commandQueue, if the client attempts commands fast enough (more than one every timeoutPerCommand ms), then self.lastWriteTime continues to get updated such that Date.now() - self.lastWriteTime will continue to be less than timeoutPerCommand and thus the original command will never time out.

Perhaps in Redis.prototype.sendCommand lastWriteTime is only updated if commandQueue.length === 1.

) {
debug('Command timed out. Pending commands: %s', commandQueueLength);
var err = new Error('Command timed out');
self.flushQueue(err, { offlineQueue: false });
self.silentEmit('error', err);
Copy link

Choose a reason for hiding this comment

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

Is emitting an error event sufficient? In my usage, I'm always supplying a callback in the form of:

redis.get(key, function (err, reply) {
    if (err) {
        // handle error
    } else {
        // handle data
    }
});

While I do also handle the error event, that handling is somewhat generic. It would be better if the callback was called with the error so that specific error handling could be invoked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the callbacks will be called besides an error event. self.flushQueue() does the trick.

self.disconnect(true);
Copy link

Choose a reason for hiding this comment

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

I assume that this will trigger an automatic reconnect to that server, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's correct.

Copy link

Choose a reason for hiding this comment

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

Good, but see my follow-up to #587. If this node was part of a cluster, retryStrategy is overridden and set to null so there would in fact be no automatic reconnect to that node.

Copy link

Choose a reason for hiding this comment

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

Per #587, there will not be any automatic reconnect in a cluster environment.

}
}, 500);
Copy link

Choose a reason for hiding this comment

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

It appears that the minimum practical value for timeoutPerCommand is 500ms. Even if I specify timeoutPerCommand = 100, the check for a timed out command occurs every 500ms and so the command time out error won't trigger as quickly as I would expect.

}

if (self.options.enableReadyCheck) {
self._readyCheck(function (err, info) {
if (err) {
Expand Down Expand Up @@ -71,6 +90,11 @@ exports.closeHandler = function (self) {
self.prevCommandQueue = self.commandQueue;
}

if (self.timeoutPerCommand) {
clearInterval(self.timeoutPerCommand);
self.timeoutPerCommand = null;
}

if (self.manuallyClosing) {
self.manuallyClosing = false;
debug('skip reconnecting since the connection is manually closed.');
Expand Down