Sitelet https://github.com/phpredis/phpredis/commit/develop
Skip to content

Commit 146ec81

Browse files
michael-grundermichael-grunder
committed
fix: Flaky test and pool reconnect logic
Fix flaky `testTime` test by reworking the pool health check logic so that a successful reconnect doesn't still throw an exception. Additionally explicitely close the connection in `testClient` since we are killing our own connection on purpose.
1 parent 1844cc5 commit 146ec81

2 files changed

Lines changed: 27 additions & 12 deletions

File tree

‎library.c‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3151,6 +3151,22 @@ static int redis_stream_detect_dirty(php_stream *stream) {
31513151
return rv == 0 ? SUCCESS : FAILURE;
31523152
}
31533153

3154+
/* Read and validate a RESP line without throwing or disconnecting. */
3155+
static int
3156+
redis_sock_gets_silent(RedisSock *redis_sock, char *buf, int buf_size, size_t *line_size)
3157+
{
3158+
if (redis_sock_get_line(redis_sock, buf, buf_size, line_size) == NULL ||
3159+
*line_size < 2 || memcmp(buf + *line_size - 2, ZEND_STRL("\r\n")) != 0)
3160+
{
3161+
return FAILURE;
3162+
}
3163+
3164+
*line_size -= 2;
3165+
buf[*line_size] = '\0';
3166+
3167+
return SUCCESS;
3168+
}
3169+
31543170
static inline zend_bool
31553171
redis_check_echo_response(RedisSock *redis_sock, char *hdr, const char *id,
31563172
size_t idlen)
@@ -3167,7 +3183,7 @@ redis_check_echo_response(RedisSock *redis_sock, char *hdr, const char *id,
31673183

31683184
/* Non-sentinel: Read and verify the ID */
31693185
return *hdr == TYPE_BULK && atoi(hdr + 1) == idlen &&
3170-
redis_sock_gets(redis_sock, buf, sizeof(buf) - 1, &len) == 0 &&
3186+
redis_sock_gets_silent(redis_sock, buf, sizeof(buf) - 1, &len) == SUCCESS &&
31713187
redis_strncmp(buf, id, idlen) == 0;
31723188
}
31733189

@@ -3202,15 +3218,16 @@ redis_sock_check_liveness(RedisSock *redis_sock)
32023218

32033219
resp_str_cat_str(&cmd, id, idlen);
32043220

3205-
/* Send command(s) and make sure we can consume reply(ies) */
3206-
if (redis_sock_write(redis_sock, ZSTR_VAL(cmd.s), ZSTR_LEN(cmd.s)) < 0) {
3221+
/* Probe only this stream, without throwing, reconnecting, or changing pool
3222+
* accounting. The caller will replace it if the probe fails. */
3223+
if (redis_sock_write_raw(redis_sock, ZSTR_VAL(cmd.s), ZSTR_LEN(cmd.s)) != ZSTR_LEN(cmd.s)) {
32073224
smart_str_free(&cmd);
32083225
goto failure;
32093226
}
32103227

32113228
smart_str_free(&cmd);
32123229

3213-
if (redis_sock_gets(redis_sock, inbuf, sizeof(inbuf) - 1, &len) < 0) {
3230+
if (redis_sock_gets_silent(redis_sock, inbuf, sizeof(inbuf) - 1, &len) == FAILURE) {
32143231
goto failure;
32153232
}
32163233

@@ -3219,13 +3236,13 @@ redis_sock_check_liveness(RedisSock *redis_sock)
32193236
redis_strncmp(inbuf, ZEND_STRL("-ERR Client sent AUTH")) == 0)
32203237
{
32213238
/* successfully authenticated or authentication isn't required */
3222-
if (redis_sock_gets(redis_sock, inbuf, sizeof(inbuf) - 1, &len) < 0) {
3239+
if (redis_sock_gets_silent(redis_sock, inbuf, sizeof(inbuf) - 1, &len) == FAILURE) {
32233240
goto failure;
32243241
}
32253242
} else if (redis_strncmp(inbuf, ZEND_STRL("-NOAUTH")) == 0) {
32263243
/* connection is fine but authentication failed, next command must
32273244
* fail too */
3228-
if (redis_sock_gets(redis_sock, inbuf, sizeof(inbuf) - 1, &len) < 0
3245+
if (redis_sock_gets_silent(redis_sock, inbuf, sizeof(inbuf) - 1, &len) == FAILURE
32293246
|| redis_strncmp(inbuf, ZEND_STRL("-NOAUTH")) != 0)
32303247
{
32313248
goto failure;
@@ -4540,8 +4557,7 @@ redis_sock_gets(RedisSock *redis_sock, char *buf, int buf_size, size_t *line_siz
45404557
return -1;
45414558
}
45424559

4543-
if(redis_sock_get_line(redis_sock, buf, buf_size, line_size) == NULL ||
4544-
*line_size < 2 || memcmp(buf + *line_size - 2, ZEND_STRL("\r\n")) != 0)
4560+
if (redis_sock_gets_silent(redis_sock, buf, buf_size, line_size) == FAILURE)
45454561
{
45464562
if (redis_sock->port < 0) {
45474563
snprintf(buf, buf_size, "read error on connection to %s", ZSTR_VAL(redis_sock->host));
@@ -4556,10 +4572,6 @@ redis_sock_gets(RedisSock *redis_sock, char *buf, int buf_size, size_t *line_siz
45564572
return FAILURE;
45574573
}
45584574

4559-
/* We don't need \r\n */
4560-
*line_size -= 2;
4561-
buf[*line_size] = '\0';
4562-
45634575
/* Success! */
45644576
return 0;
45654577
}

‎tests/RedisClusterTest.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,9 @@ public function testClient() {
495495

496496
/* Kill our own client! */
497497
$this->assertTrue($this->redis->client($key, 'kill', $addr));
498+
499+
/* Do not return a connection awaiting the server's close to the pool. */
500+
$this->redis->close();
498501
}
499502

500503
public function testTime() {

0 commit comments

Comments
 (0)