Retry connection on socket error #106
Conversation
|
related to #57 and yiisoft/yii2#6356 |
Connection.php
Outdated
| while ($retries-- >= 0) { | ||
| try { | ||
| fwrite($this->_socket, $command); | ||
| return $this->parseResponse(implode(' ', $params)); |
There was a problem hiding this comment.
Should $this->retries be reset here on success?
There was a problem hiding this comment.
it has not changed, I call -- on $retries only.
There was a problem hiding this comment.
I mean currently $retries is shared among all queries and I think it should be per query.
There was a problem hiding this comment.
it is per query currently. One call to "executeCommand" is one query.
There was a problem hiding this comment.
Good morning myself :( I was looking at $retries-- seeing $this->retries--. Sorry.
Connection.php
Outdated
| @@ -647,8 +653,20 @@ public function executeCommand($name, $params = []) | |||
| } | |||
|
|
|||
| \Yii::trace("Executing Redis Command: {$name}", __METHOD__); | |||
| if ($this->retries > 0) { | |||
There was a problem hiding this comment.
$tries = $this->retries + 1;
while ($tries--) {
try {
} catch () {
}
}This might be better, and also removed the duplicated codes (fwrite and parseResp)
There was a problem hiding this comment.
looks better code wise, but it has the problem, that even if retry is disabled, the connection is reset on the first error.
cebe
changed the title
| $db = $this->getConnection(false); | ||
| $db->configSet('timeout', 1); | ||
| $this->assertTrue($db->ping()); | ||
| sleep(1); |
There was a problem hiding this comment.
I wonder if these tests pass, since you have a timeout of 1 (sec?) and you sleep 1 seconds, plus a few microseconds for code-execution... Wouldn't it be better to lower this to 0.5?
There was a problem hiding this comment.
We could change it if needed but I have not seen any issues with this, neither locally nor on travis. Will improve tests a bit.
tests/RedisConnectionTest.php
Outdated
| sleep(1); | ||
| $this->assertTrue($db->ping()); | ||
| sleep(2); | ||
| $this->assertTrue($db->ping()); |
There was a problem hiding this comment.
I would add another two lines with sleep(3) and the expected exception from above.
Connection.php
Outdated
| fwrite($this->_socket, $command); | ||
| if ($this->retries > 0) { | ||
| $tries = $this->retries; | ||
| while ($tries-- >= 0) { |
There was a problem hiding this comment.
This looks incorrect. If $tries = 1 then $tries-- will return 1. So $this->retries = 1 will result 2 retries instead of 1.
It should be while ($tries-- > 0) or use pre-increment.
There was a problem hiding this comment.
if $tries = 1, the while loop will run once. that means on first exception, it will try to reconnect, then not enter the while loop again so the second command will run normally. This will be 2 "tries" but one of them is the retry ;)
Imo that is correct and expected. $this->retries = 0 means one command execution, $this->retries = 1 means two in case of error.
There was a problem hiding this comment.
if $tries = 1, the while loop will run once.
It will run twice: https://3v4l.org/NObVY
Or I'm missing something obvious. :P
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems I mixed up the combination of -- before or after and the > and >= usage.
--$triesdecrements first and then checks against 0$tries--checks against 0 and then decrements
Will add a test to verify the number of trials.
There was a problem hiding this comment.
I'd make that explicit by separating decrement and condition.
currently does not handle fwrite failure, only socket read
|
Travis fails. |
|
Oh, that was because I rebased on master. Tests for this PR are passing. |
Possible solution for #91.
currently does not handle fwrite failure, only socket read.