Hello, I have found some bugs with the original commit that adds redis_pipe_cmd. If a command to the server fails, this may leave the redisContext in an invalid state, and this means that a reconnect to the server needs to be performed.
The problem is that when reconnecting, a new context is created, and all the contents of old redisContext is lost, including the hiredis output buffer (obuf) which stored the piped commands. This means that all the commands are lost, and when a second retry is made this contains no commands and may fail, leaving the redisContext in an invalid state. This can in some cases break the REDIS pipelining feature, and cause all commands sent using redis_pipe_cmd()/redis_execute() to fail from that point onwards.
The solution is to make sure that the connection is active when adding commands to the hiredis output buffer. For this, commands are stored in a separate buffer, and are only added to the redisContext when actually sending the command (when calling redis_execute). This way it is possible to see if any errors exist by checking the err member of the redisContext structure before appending the commands, and also commands are not lost since they are stored separately and if reconnecting the new commands can be appended to the output buffer of the new redisContext.
Commands are stored in buffers using the sds format hiredis offers (by calling redisFormatCommand function offered by hiredis). This way they can be directly appended to the output buffer using redisAppendFormattedCommand.
There is a problem on older hiredis versions (older than 0.12) because redisAppendFormattedCommand was not exported to the library API, so for older hiredis version the buffers are appended in our code.
We tested the code using hiredis library version 0.10.1 and 0.12.1 and there is no problem with this approach.
In the second commit we fixed a memory leak when re-using reply ids in the same pipeline command sequence occurs.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1115
-- Commit Summary --
* ndb_redis: fix connection problems with pipelining
* ndb_redis: fix memory leak
-- File Changes --
M src/modules/ndb_redis/redis_client.c (116)
M src/modules/ndb_redis/redis_client.h (11)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1115.patchhttps://github.com/kamailio/kamailio/pull/1115.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/1115
Module: kamailio
Branch: master
Commit: b8b900a960e315e15f6ff2947e2ff64881882e8b
URL: https://github.com/kamailio/kamailio/commit/b8b900a960e315e15f6ff2947e2ff64…
Author: Kamailio Dev <kamailio.dev(a)kamailio.org>
Committer: Kamailio Dev <kamailio.dev(a)kamailio.org>
Date: 2017-05-02T16:16:21+02:00
modules: readme files regenerated - ndb_redis ... [skip ci]
---
Modified: src/modules/ndb_redis/README
---
Diff: https://github.com/kamailio/kamailio/commit/b8b900a960e315e15f6ff2947e2ff64…
Patch: https://github.com/kamailio/kamailio/commit/b8b900a960e315e15f6ff2947e2ff64…
---
diff --git a/src/modules/ndb_redis/README b/src/modules/ndb_redis/README
index 1c6027e..280a6f4 100644
--- a/src/modules/ndb_redis/README
+++ b/src/modules/ndb_redis/README
@@ -51,7 +51,7 @@ Carsten Bock
4.1. redis_cmd(srvname, command, ..., replyid)
4.2. redis_pipe_cmd(srvname, command, ..., replyid)
- 4.3. redis_execute([srvname])
+ 4.3. redis_execute(srvname)
4.4. redis_free(replyid)
List of Examples
@@ -87,7 +87,7 @@ Chapter 1. Admin Guide
4.1. redis_cmd(srvname, command, ..., replyid)
4.2. redis_pipe_cmd(srvname, command, ..., replyid)
- 4.3. redis_execute([srvname])
+ 4.3. redis_execute(srvname)
4.4. redis_free(replyid)
1. Overview
@@ -215,7 +215,7 @@ modparam("ndb_redis", "cluster", 1)
4.1. redis_cmd(srvname, command, ..., replyid)
4.2. redis_pipe_cmd(srvname, command, ..., replyid)
- 4.3. redis_execute([srvname])
+ 4.3. redis_execute(srvname)
4.4. redis_free(replyid)
4.1. redis_cmd(srvname, command, ..., replyid)
@@ -293,17 +293,13 @@ if(redis_cmd("srvN", "HMGET foo_key field1 field3", "r")) {
If cluster parameter is set to 1, this function will log an error and
do nothing.
-4.3. redis_execute([srvname])
+4.3. redis_execute(srvname)
Sends commands to REDIS server identified by srvname. Commands are
added with redis_pipe_cmd function, and will be stored for an existing
REDIS server. When this function is called all the commands will be
sent in a single message to the REDIS server.
- If this function is called without any parameters, it will iterate
- through all existing servers and send the existing pipelined commands
- for them.
-
When using redis_cmd together with redis_pipe_cmd it is recommended
that a call to redis_execute is made before calling redis_cmd in case
there are pipelined commands, otherwise when calling redis_cmd, if
@@ -321,14 +317,14 @@ After several redis command calls:
redis_pipe_cmd("srvB", "SET ruri $ru", "r2");
- redis_pipe_cmd("srvC", "GET foo", "r3");
+ redis_pipe_cmd("srvB", "GET foo", "r3");
Send the data and retrieve the results:
redis_execute("srvA"); //send command to srvA and wait for reply. Store
the reply in r1
- redis_execute(); //send remaining commands (the set to srvB and get to s
-rvC), wait for replies, and store the replies in r2 and r3
+ redis_execute("srvB"); //send command to srvA and wait for reply. Store
+the replies in r2 (for SET ruri $ru) and r3 (for GET foo)
Using both redis_cmd and redis_pipe_cmd:
redis_pipe_cmd("srvA", "SET foo bar", "r1");