redis / hiredis

Minimalistic C client for Redis >= 1.2
BSD 3-Clause "New" or "Revised" License
6.21k stars 1.81k forks source link

Bug When Clearing Reader Input Buffer #584

Closed kgodara closed 6 years ago

kgodara commented 6 years ago

I currently am using Hiredis to pipeline commands in batches and then send them to a local redis instance, however in order to avoid having to parse each server reply individually using redisGetReply() (pipelining thousands of commands), it would be more efficient to be able to completely clear the reader input buffer and resume pipelining commands for the next batch. However, I am running into the error below after successfully sending the first two batches of commands. Error occurs upon calling freeReplyObject(). arby_redis(45021,0x70000ca86000) malloc: *** error for object 0x7fb7d3f1e0c0: pointer being freed was not allocated *** set a breakpoint in malloc_error_break to debug Abort trap: 6

Printing the errstr from my RedisContext outputted: Protocol error, got "\n" as reply type byte

I have verified that the first two batches of commands are being successfully sent and executed. Below is a stripped down version of my code:

   redisContext *context = redisConnect("127.0.0.1", 6379);
    redisReply *reply;
    context->reader->maxbuf = 0;
    for (i = 0 ; i < getArraySize(data) ; i++)
    {
        sprintf(cmd, "HMSET example:%s field1 %s field2 %s", id, field1, field2);
        redisAppendCommand(context, cmd);
        memset(cmd, 0, MAX_CMD_SIZE);

        sprintf(cmd, "ZADD example %f %s", score, element);
        redisAppendCommand(context, cmd);
        memset(cmd, 0, MAX_CMD_SIZE);

        cmd_num += 2;

        if (cmd_num >= MAX_CMD_QUEUE) {
            int return_val = redisGetReply(context, (void **)&reply);
            if(return_val == REDIS_ERR) {
                printf("%d\n", context->err);
                printf("%s\n", context->errstr);
            }
            freeReplyObject(reply);
            // Try to empty the reader buffer
            sdsfree(context->reader->buf);
            context->reader->buf = sdsempty();
            context->reader->pos = context->reader->len = 0;
            context->reader->ridx = -1;
            context->reader->err = 0;
            context->reader->errstr[0] = '\0';
            cmd_num = 0;
        }
    }

I am also confident that the commands themselves are structured correctly, as I have tested reading and parsing every single reply in the input stream, and the program executes without error (at cost of performance).

The exact same error occurs when using the code below to attempt to reset the input buffer: redisReaderFree(context->reader); context->reader = redisReaderCreate();

Any insight would be appreciated! Thanks for your time!

michael-grunder commented 6 years ago

Hi,

I was able to see this issue myself but I didn't investigate too deeply to find the specific cause. My guess is that you hit this issue when you clear the local buffer but there is still more data "to be read" over the wire. You truncate the data already read, but since there is more, hiredis is now out of sync (hence the 'X as reply type byte error').

If you don't mind, I'd like to suggest a totally different strategy to accomplish what you're trying to do.

Since version 3.2, Redis has had the CLIENT REPLY command. This will let you instruct Redis to stop sending replies at all to you.

Using this command you can do something like this:


/* Note that Redis will NOT reply to this, so don't wait for it */
redisAppendCommand(context, "CLIENT %s %s", "reply", "off");

/* Dump our commands */
for (i = 0; i < commands; i++) {
    redisAppendCommand(context, "SET %s %s", keys[i], values[i]);
}

/* Wake up replying (this also lets us wait for the looped commands to finish */
if (redisGetReply(context, (void**)&reply) == REDIS_OK) {
    /* Success */
} else {
    /* Handle error... */
}

A couple of other things regarding your usage of hiredis. If you already know all of this then feel free to disregard! 😄

In your program you are appending commands as a single string like this:

sprintf(cmd, "HMSET example:%s field1 %s field2 %s", id, field1, field2);
redisAppendCommand(context, cmd);

This is going to lead to weird problems if your values ever have spaces. redisAppendCommand is variadic, so you can accomplish that like this:

redisAppendCommand(context, "HMSET example:%s field1 %s field2 %s", id, field1, field2)

Also look into using redisAppendCommandArgv if your goal is maximum performance. It's more work on your end but often you will already know things like argument lengths!

Cheers! Mike

michael-grunder commented 6 years ago

Hi, since this isn't a bug in hiredis I'm going to close the issue. If you're still having trouble with the problem feel free to comment here and I'll try to help.

Cheers Mike