Skip to content

Commit

Permalink
Make our timeout or response error handling more explicit.
Browse files Browse the repository at this point in the history
Although a -1 return value from cluster_check_response is likely
a timeout, it is not the only possibility, so handle the loop
timeout and error response in distinct ways.
  • Loading branch information
michael-grunder committed Oct 17, 2018
1 parent 27df922 commit 07ef7f4
Showing 1 changed file with 17 additions and 8 deletions.
25 changes: 17 additions & 8 deletions cluster_library.c
Original file line number Diff line number Diff line change
Expand Up @@ -1419,8 +1419,11 @@ PHP_REDIS_API short cluster_send_command(redisCluster *c, short slot, const char
return -1;
}

/* Now check the response from the node we queried. */
/* Check response and short-circuit on success or communication error */
resp = cluster_check_response(c, &c->reply_type TSRMLS_CC);
if (resp == 0 || resp == -1) {
break;
}

/* Handle MOVED or ASKING redirection */
if (resp == 1) {
Expand All @@ -1439,22 +1442,28 @@ PHP_REDIS_API short cluster_send_command(redisCluster *c, short slot, const char
}
}

/* We're timed out if cluster_check_response returned -1, or if the
* response was non-zero and we've been in the loop too long */
timedout = resp == -1 || (resp && c->waitms ? mstime() - msstart >= c->waitms : 0);
} while (resp != 0 && !c->clusterdown && !timedout);
/* See if we've timed out in the command loop */
timedout = c->waitms ? mstime() - msstart >= c->waitms : 0;
} while (!c->clusterdown && !timedout);

// If we've detected the cluster is down, throw an exception
if (c->clusterdown) {
zend_throw_exception(redis_cluster_exception_ce,
"The Redis Cluster is down (CLUSTERDOWN)", 0 TSRMLS_CC);
return -1;
} else if (timedout) {
} else if (timedout || resp == -1) {
// Make sure the socket is reconnected, it such that it is in a clean state
redis_sock_disconnect(c->cmd_sock, 1 TSRMLS_CC);

zend_throw_exception(redis_cluster_exception_ce,
"Timed out attempting to find data in the correct node!", 0 TSRMLS_CC);
if (timedout) {
zend_throw_exception(redis_cluster_exception_ce,
"Timed out attempting to find data in the correct node!", 0 TSRMLS_CC);
} else {
zend_throw_exception(redis_cluster_exception_ce,
"Error processing response from Redis node!", 0 TSRMLS_CC);
}

return -1;
}

/* Clear redirection flag */
Expand Down

2 comments on commit 07ef7f4

@yatsukhnenko
Copy link
Member

Choose a reason for hiding this comment

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

Why do you use if (resp == 0 || resp == -1) { and not if (resp <= 0) {? 😄

@michael-grunder
Copy link
Member Author

@michael-grunder michael-grunder commented on 07ef7f4 Oct 17, 2018

Choose a reason for hiding this comment

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

¯\(ツ)

Please sign in to comment.