From 221b7f2d6c822bc2fccf927f5ded5c4788da8150 Mon Sep 17 00:00:00 2001 From: Rian McGuire Date: Mon, 29 Jul 2024 14:19:12 +1000 Subject: [PATCH] Don't flush hiredis write buffer during reads HiredisConnection#write and #write_multi already call flush after writing, so this is unnecessary. This fixes a thread safety issue when PubSub is used across multiple threads. --- .../redis_client/hiredis/hiredis_connection.c | 61 ++++++------------- 1 file changed, 18 insertions(+), 43 deletions(-) diff --git a/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c b/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c index 14accbc..dc91296 100644 --- a/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c +++ b/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c @@ -719,7 +719,6 @@ static VALUE hiredis_flush(VALUE self) { static int hiredis_read_internal(hiredis_connection_t *connection, VALUE *reply) { void *redis_reply = NULL; - int wdone = 0; // This struct being on the stack, the GC won't move nor collect that `stack` RArray. // We use that to avoid having to have a `mark` function with write barriers. @@ -737,56 +736,32 @@ static int hiredis_read_internal(hiredis_connection_t *connection, VALUE *reply) return HIREDIS_FATAL_CONNECTION_ERROR; // Protocol error } - if (redis_reply == NULL) { - /* Write until the write buffer is drained */ - while (!wdone) { - errno = 0; - - if (hiredis_buffer_write_nogvl(connection->context, &wdone) == REDIS_ERR) { - return HIREDIS_FATAL_CONNECTION_ERROR; // Socket error - } - - if (errno == EAGAIN) { - int writable = 0; - - if (hiredis_wait_writable(connection->context->fd, &connection->write_timeout, &writable) < 0) { - return HIREDIS_CLIENT_TIMEOUT; - } + /* Read until there is a full reply */ + while (redis_reply == NULL) { + errno = 0; - if (!writable) { - errno = EAGAIN; - return HIREDIS_CLIENT_TIMEOUT; - } - } + if (hiredis_buffer_read_nogvl(connection->context) == REDIS_ERR) { + return HIREDIS_FATAL_CONNECTION_ERROR; // Socket error } - /* Read until there is a full reply */ - while (redis_reply == NULL) { - errno = 0; + if (errno == EAGAIN) { + int readable = 0; - if (hiredis_buffer_read_nogvl(connection->context) == REDIS_ERR) { - return HIREDIS_FATAL_CONNECTION_ERROR; // Socket error + if (hiredis_wait_readable(connection->context->fd, &connection->read_timeout, &readable) < 0) { + return HIREDIS_CLIENT_TIMEOUT; } - if (errno == EAGAIN) { - int readable = 0; - - if (hiredis_wait_readable(connection->context->fd, &connection->read_timeout, &readable) < 0) { - return HIREDIS_CLIENT_TIMEOUT; - } - - if (!readable) { - errno = EAGAIN; - return HIREDIS_CLIENT_TIMEOUT; - } - - /* Retry */ - continue; + if (!readable) { + errno = EAGAIN; + return HIREDIS_CLIENT_TIMEOUT; } - if (redisGetReplyFromReader(connection->context, &redis_reply) == REDIS_ERR) { - return HIREDIS_FATAL_CONNECTION_ERROR; // Protocol error - } + /* Retry */ + continue; + } + + if (redisGetReplyFromReader(connection->context, &redis_reply) == REDIS_ERR) { + return HIREDIS_FATAL_CONNECTION_ERROR; // Protocol error } }