R crash on Redis server loss about rcppredis HOT 21 CLOSED

Not sure how much I really care. Redis is a generally always up quality and when it is down access from R may be the least worry.

Pull requests welcome. I am unlikely to do this myself.

from rcppredis.

I might take this one on; I'm loathe to see an entire R session crash because of an intermittent network error. But, I suppose if I did take it on we should also add some of the benchmarking code to the testing framework - just to be sure that none of the changes result in a notable performance hit.

from rcppredis.

Easy -- prefix each user-space command with a ping :)

We would at least one line to each function emitting a request to Redis. Worth it?

from rcppredis.

Well, the ping will tell you that it is up at the time of the ping, but not that it was actually up/connected correctly at the time of your request. It also adds an additional round-trip to any transaction. Clearly NVG. Instead, I was thinking instead of wrapping redisCommand and redisCommandArgv with a safety check that a NULL wasn't received as the reply. It would mean refactoring the code to use the NULL safe versions of those functions.

I quickly banged out the following:

    void nullReplyCheck(redisReply *reply) {
        if (reply == NULL) {
            Rcpp::stop("Recieved NULL reply; potential connection loss with Redis");
        }
    }

    void *redisCommandNULLSafe(redisContext *c, const char *format, ...) {
        va_list myargs;
        va_start(myargs, format);

        redisReply *reply = 
            static_cast<redisReply*>(redisvCommand(c, format, myargs));
        nullReplyCheck(reply);
        return reply;
    }

    void *redisCommandArgvNULLSafe(redisContext *c, int argc, const char **argv, const size_t *argvlen) {
        redisReply *reply = 
            static_cast<redisReply*>(redisCommandArgv(c, argc, argv, argvlen));
        nullReplyCheck(reply);
        return reply;
    }

It seems to pass tests and survives the described example. What I don't know is if it would recover from a transitory loss of the connection. I haven't played around with toxiproxy yet, maybe @richfitz could do so and let us know if this solution saves us from anything other than the test example.

from rcppredis.

Nice. I had the same idea but was in a rush and didn't see / remember redisCommand ...

from rcppredis.

I stupidly have been making my edits to RcppRedis on my master branch. Once we've resolved the pending pull request, then I'll submit this one for consideration.

from rcppredis.

Replicated the test as described here https://github.com/richfitz/toxiproxyr. Confirmed that the proposed code works under that case as well.

from rcppredis.

I stupidly have been making my edits to RcppRedis on my master branch.

We've all been there. Maybe save the files by hand and do a git reset ? It would be cleaner.

from rcppredis.

@eddelbuettel You can probably close this issue.

from rcppredis.

Just to be plain: because your PR addressed it?

from rcppredis.

Yes; I believe this issue is fully addressed by #23. It might be nice if @richfitz would confirm; but I feel reasonably certain in my testing results.

from rcppredis.

I actually was able to experience this one in the wild while torturing a Redis server. The modifications ended up working as expected. Though, it got me wondering about whether trying to automatically rebuild the connection at least once would make sense before restoring to throwing an error.

from rcppredis.

One could. But then: try once? Twice? N+1 times? At some point you gotta error out.

from rcppredis.

True, and I kind of suspect that retrying the connection wouldn't have solved anything in the case I experienced; although it might protect users who incorrectly use a fork of the connection argument. In the latter vein, I was thinking of defaulting to one attempt after a 1 second delay, but perhaps adding yet another configurable parameter to init.

EDIT: Actually a hard limit of 1 would be easiest. Promising more than one would require thinking carefully about how to handle errors when making the initial connection / writing out separate reconnection code.

from rcppredis.

I can live with trying once. The server may just be away 'temporarily' (even for an upgrade). It would be a nice to have, but I don't think it is a priority.

(In general the edge-case handling in RcppRedis is of course rudimentary. We make day. "One day" @richfitz will gift us a real package on CRAN and then we all switch anyway :) Until then, hack mode...)

from rcppredis.

In addition to not being on CRAN one of my reservations about redux was not knowing how it would do in speed compared to RcppRedis especially on serialization/deserialization; for some reason I ended up with the misconception that was happening using methods similar to rredis. At any rate, I was pleasantly surprised, redux compares favorably to RcppRedis.

from rcppredis.

Nice benchmarks! And thumbs up for empirics. I only glanced (about to head out) but you also included serialization cost, right?

from rcppredis.

I believe I did account for the serialization costs, but I always welcome peer review.

from rcppredis.

I'm sorry that this comment is off-topic from the original issue posting.

Note that the first example serializes the object 1000 times in the
hiredis and rredis loops, but only once in the redux benchmark loop.

But to be fair I don't think that matters too much. I was a bit surprised
that rredis was ~ 10x slower here, so I made some easy performance optimizations to the rredis github master branch. For comparison, I now get (on my desktop Ubuntu quad-core PC running R-3.2.3):

       test replications elapsed relative
1 hiredis()         1000   0.110    1.000
2  rredis()         1000   0.281    2.555

for the file adapted from http://rpubs.com/rpierce/simpleBenchmarkRedux (and slightly faster yet if you uncomment the pipelining lines):

library(RcppRedis)
library(rredis)
library(rbenchmark)

data(trees)
fit <- lm(log(Volume) ~ log(Girth) + log(Height), data=trees)

redis <- new(Redis)
hiredis <- function() redis$set("fits", fit)
rredis <- function() redisSet("fits2", fit)
redisConnect()

#redisSetPipeline(TRUE)
res <- benchmark(hiredis(), rredis(), replications=1000)[,1:4]
#redisSetPipeline(FALSE)
print(res)

The rest of the ~ 2x difference with RcppRedis is not easy however. Much of it is in tryCatch for things like interrupts of transfers (important in many real-world settings), other problems (too large of payloads), etc.

Again, sorry for the diversion.

On 12/11/15, drknexus [email protected] wrote:

I believe I did account for the serialization costs, but I always welcome
peer review.

---

Reply to this email directly or view it on GitHub:
#20 (comment)

from rcppredis.

A welcome diversion; I'm not sure there is a good forum functionality in which we could have discussed this otherwise.

@bwlewis Are you sure the first example serializes the object 1000 times in the hiredis and rredis loops, but only once in the redux benchmark loop? I'm not seeing it. The difference is that both rredis and RcppRedis implicitly do the serialization whereas with redux you have to manually handle it to coerce the input to $SET.

Regardless, I'm thrilled to hear that you found some places for performance enhancement in rredis.

from rcppredis.

Closing per request earlier in this issue.

from rcppredis.