Added blpop and adjusted to signed 64 bit numbers

[etcimon]

Because we're finalizing the Redis API, I thought it would be important to make some better decisions.

First, it's very unsafe for the API to use size_t b/c on i386 platforms forcing 32 bit integers could result in data loss considering Redis sends a string of a 64 bit integers always.

I added bblpop as a blocking list pop and a non-blocking list blpop as a task with concurrency. An issue I pointed out earlier was that redis was not precise on its timeout. The suggestion was to close the connection after 10 seconds, but using a task the best workaround would be to run :

auto value = redis.blpop!(string[])(10.seconds, "list1", "list2").receiveTimeout!string(10.seconds);
redis.lpush("list1", "1"); // satisfies the receive loop in blpop

This way, the connection will be recycled and the timeout ends nearer to 10s rather than 10.66s+.

I'd also like to suggest changing the template parameters, ie. lindex(T : E[], E) should be lindex(T) b/c there would be more logic requesting the response type rather than specifying the underlying range.

[s-ludwig]

Good point with the size, but should it be long or ulong?

I don't like the bblpop. "blpop" means blocking pop, so it should, well, block. The asynchronous version also doesn't really provide enough value to warrant an extra method (btw. it sends to the wrong task). Generally the goal should be to make developers aware of runTask as a standard means to achieve such effects instead of providing wrappers for one-liners. listen is an exception here, because it will supposedly be the expected behavior in most cases.

The lpush method to force a timeout I guess can't work in general, because it will influence other clients and is prone to race conditions.

Regarding, E[] vs. T. Aren't all elements currently handled as either ubyte[] or string? I think everything else (serialization/deserialization of other types) should better be left to the user in that case. But I don't really have an overview of how Redis is organized there.

---

What would possibly make sense for blpop though, would be to implement a precise timeout like this:

T blpopPrecise(Duration timeout, string[] keys...)
{
    if (timeout <= 0.seconds) return request!T("blpop", keys, 0);
    auto t = Task.getThis(); // stored in heap closure
    bool receiving = true; // stored in heap closure
    runTask({
        // round up the duration to avoid premature timeouts
        auto ret = request!T("blpop", keys, (timeout + 999.msecs).total!"seconds");
        if (receiving) t.send(ret);
        else lpush(ret); // hmm... this is wrong, is there anything to push to the end of a list instead?
    });
    T ret; // return null in case of a timeout
    if (receiveTimeout(timeout, (T val) { ret = val; });
    receiving = false;
    return ret;
}

But due to the additional cost caused be the heap closure, I'd suggest to make this an additional method instead of the only one.

[etcimon]

Regarding, E[] vs. T. Aren't all elements currently handled as either ubyte[] or string?

Ok so API calls are intentionally restricted to a response range & disallow individual values.

hmm... this is wrong, is there anything to push to the end of a list instead?

That would be rpush. The "shared" bool idea is ideal

should it be long or ulong?

"the returned integer is guaranteed to be in the range of a signed 64 bit integer."

• http://redis.io/topics/protocol#integer-reply

instead of providing wrappers for one-liners.

It's not so bad use one-liners to secure an async api that might eventually implement responding through promises (with futures which wrap around the Task type, as is on both ends.. I'm still thinking about that).

[s-ludwig]

That would be rpush. The "shared" bool idea is ideal

Oh okay... and I thought "lpush" meant "list push" all the time :)

It's not so bad use one-liners to secure an async api that might eventually implement responding through promises (with futures which wrap around the task type on both ends.. I'm still thinking about that).

But especially if there is a planned concept that will supersede such an API it makes no sense to introduce it now. Only APIs that are supposed to stay forever should be added now. Especially since the future system is a relatively simple concept (i.e. very quick to add after some design issues have been worked out), so the life time of such a function would be very limited.

[etcimon]

Oh okay... and I thought "lpush" meant "list push" all the time :)

lol I understand that feeling too well

the future system is a relatively simple concept (i.e. very quick to add after some design issues have been worked out), so the life time of such a function would be very limited.

Yes indeed.

I think this pull request can be reviewed once again, it might break some installations on vibe.d ~master which use redis but, size_t won't do!

[s-ludwig]

Thanks, as far as I can tell everything looks good.

I think this pull request can be reviewed once again, it might break some installations on vibe.d ~master which use redis but, size_t won't do!

Agreed, this is too important to not fix and there is no obvious deprecation path in this case.

[etcimon]

I was exploring const-correctness and thinking, shouldn't the RedisClient method return types all use immutable storage, and the parameters all have the in specifier?

[s-ludwig]

As far as I can see, most parameters/return values are POD types, so they wouldn't benefit from const/immutable. Then there are some string or string[]... parameters, which are already immutable because string is immutable. And then there are the methods taking generic types as parameters and those should also already accept const() values. The only different case is for RedisReply return values, but this is kind of a special case because the reply has to be modified while reading out the response. Any other cases?