Retries per request limit

[Jabher]

I'm migrating to ioredis from node_redis and cannot find max_attempts feature analog, is this about me bad in searching?
It's limit for retries per each request after which it will just fail.

I can implement it on my own as a wrapper but I need to stop requests so that they will not continue to try requesting and this feature is also missing.

[luin]

https://github.com/luin/ioredis#auto-reconnect

What's you need is retryStrategy.

[Jabher]

It's not about retry strategy. Retry strategy is used to reconnect, right?
I'm asking about redis commands, not redis connections. E.g. webserver is
expected to response in 60 seconds or less otherwise data from redis is
useless.

I do not want to store requests in offline queue, I need to drop them in 5
seconds if redis connections are unavailable and apply another strategy
from business logic. Failproof, man. I know redis is stable but what if
some weirdo infrastructure engeneer drops my cluster. (Already happened
once BTW)

I can do Promise.race([redis.get(token), timeoutRejectPromise(5000)]), but I still want to clear the offline
queue of the lib so that when connection will reestablish they will not get
data, occupy the network traffic and so on.

On Wed, Jun 3, 2015, 18:29 Zihua Li notifications@github.com wrote:

Closed #61 #61.

—
Reply to this email directly or view it on GitHub
#61 (comment).

[luin]

max_attempts in node_redis is used for reconnection, not for per request, just like retryStrategy in ioredis. You can set enableOfflineQueue to false to disable offline queue.

You may handle retrying and timeout of a request on the application side.

[AVVS]

I was thinking that it actually makes sense to have some sort of TTL on the request. Idea is that some requests mutate state and some don't, and those that mutate, especially in microservice environments must be controlled. Because when there is a message bus like rabbitmq, services don't really know about each other, whether something had died or not, therefore it would be a great option to have a built-in function, that could pull command out of offline queue after specified time if it wasn't executed. @luin do you think it's possible to do so?

Ie use case that I assume should be the following:

redis.set('somekey', 'somevalue').timeout(5000).then(<ok>, <err>)

what really must be implement is that when we timeout is called, the cancel is issued (bluebird has cancellable promises as far as I remember) and the library takes care of pulling the command out of offline queue as long as it's there

what do you think?

[luin]

Currently timeout method works because when the command timed out, bluebird rejects the promise. However since not knowing the fact, ioredis will still try to resend the command until get a reply from redis server, which would be ignored though because the promise has already been rejected.

It's not hard to add the feature of per-request retries limiting with the API something like redis. maxAttempts(3).set('somekey', 'somevalue') and redis.timeout(5000).set('somekey', 'somevalue'). Aware of the limiting, ioredis is able to pulling the command out of offline queue as long as the limiting is reached.

However I'd like to see a better API since redis.timeout(5000).set(k, v) may mislead people into thinking timeout is a redis command.

[AVVS]

well, there is .pipeline() / multi() + .exec().

What if there is something similar in terms of semantics? Ie enter limiting mode and then exec to set it in motion. It might be a bit verbose though

[Jabher]

Maybe timeout/retries count can be used like a .pipeline option? As far as
it's rare but reasonably needed case it can have a bit complex api.

Or just cancelable promise can be implemented and all the logic of
retries/timeouts will be left for users responsibility, just note that
promise is actually cancelable is required.

On Fri, Jun 12, 2015, 20:17 Vitaly Aminev notifications@github.com wrote:

well, there is .pipeline() / multi() + .exec().

What if there is something similar in terms of semantics? Ie enter
limiting mode and then exec to set it in motion. It might be a bit verbose
though

—
Reply to this email directly or view it on GitHub
#61 (comment).

[luin]

Does adding an option of maxAttemptsPerRequest to the Redis constructor works for you? @Jabher

[Jabher]

Sorry, won't be able to test it until Tuesday, I check it as soon as I can

On Sat, Jun 13, 2015, 08:00 Zihua Li notifications@github.com wrote:

Does adding an option of maxAttemptsPerRequest to the Redis constructor
works for you? @Jabher https://github.com/Jabher

—
Reply to this email directly or view it on GitHub
#61 (comment).

[luin]

It haven't been implemented. I'm just wondering whether it would solve your problem to add a maxAttemptsPerRequest option. :-)

[Jabher]

I definitely have to sleep more :) Good question, I'll have a discussion with our guys.
Actually, we discussed my usecase with devs team we figured out that we need to save data even after timeout, so using only bluebird .timeout() is correct scenario. But in case we would need dropping the requests - that option would be enough. But I'd rather implement just cancellable promise anyway as far as per-request customisation is more flexible than per-connection. E.g. for my case I would rather prefer to drop read requests and leave write requests working. But for me it would just good option than killer-feature that I need to implement.

[AVVS]

If you leave unattended write requests that mutate state you can introduce race condition vulnerabilities - thats the problem here

On Jun 13, 2015, at 1:23 PM, Vsevolod Rodionov notifications@github.com wrote:

I definitely have to sleep more :) Good question, I'll have a discussion with our guys.
Actually, we discussed my usecase with devs team we figured out that we need to save data even after timeout, so using only bluebird .timeout() is correct scenario. But in case we would need dropping the requests - that option would be enough. But I'd rather implement just cancellable promise anyway as far as per-request customisation is more flexible than per-connection. E.g. for my case I would rather prefer to drop read requests and leave write requests working. But for me it would just good option than killer-feature that I need to implement.

—
Reply to this email directly or view it on GitHub #61 (comment).

[Jabher]

I know. It's only HMSETs and Redis is used as a caching reflection for other database.

[AVVS]

Yeah, but I’m concerned about a general solution for this, hence the thought

On Jun 13, 2015, at 1:41 PM, Vsevolod Rodionov notifications@github.com wrote:

I know. It's only HMSETs and Redis is used as a caching reflection for other database.

—
Reply to this email directly or view it on GitHub #61 (comment).

[joshualo]

I'm experiencing a related issue where a bug in my code was sending a SET command with an empty string as the key. In the version of nutcracker (0.3.0) that I'm using, an empty key causes the server to close the connection. ioredis retries and reconnects to the server but when the request is reattempted, the cycle is triggered infinitely making the server unresponsive.

I have fixed the bug in my code and have upgraded nutcracker to version 0.4.1 which accepts empty keys, but something like a retry limit is needed to handle the rare cases where a request causes the connection to close.

[luin]

I would prefer adding maxAttemptsPerRequest and timeoutPerRequest options. Any other ideas?

[AVVS]

these options are good, but they should be usable together and probably should be located in a generic configuration. Also a good idea would be so that can accept both function and a number. When there is function - its invoked with args for the command (include the command name) and when smth that is =< 0 returned - then we have infinite attempts / no timeouts - otherwise use number as a limit

[luin]

@AVVS That sounds good.

[shaharmor]

+1 for this. Would greatly help control the priority of requests.

[focomoso]

Is there any news here? I have what seems to be a very common use case that could use this timeoutPerRequest option. (redis-rb has something similar that they call read_timeout and write_timeout). We're using redis as a mem cache. We do a get on redis and if what we're looking for isn't there, we pull it from an upstream service. The problem comes when the connection to redis is lost. We have to wait for the get to timeout (10 seconds) before we know there's a problem and can switch to the upstream.

[luin]

You may disable the retryStrategy option so that once the connection to redis is lost, all commands will be rejected immediately.

If you're using promise, you can use timeout method:

let data;
try {
  data = await redis.get('key').timeout(500);
} catch (err) {
  data = await mysql.query(/* ... */);
}

timeoutPerRequest option is the top priority one in my todo list though.

[focomoso]

Thanks @luin - I ended up going with the timeout promise method, but a timeoutPerRequest would be great too. Make things cleaner.

[nicholasf]

@luin how far are you with the timeoutPerRequest logic? I'm wondering if there's any point in us attempting a solution over here. We need this functionality too. :)

Great work on the project!

[luin]

@nicholasf It's not easy to support timeoutPerRequest option efficiently. The alternative way is to support maxRetriesPerRequest, which can be implemented on the client side by flushing the offlineQueue in the retryStrategy, so I'm not sure whether this feature need to be implemented.

[nicholasf]

@luin ok, thanks. Will look into maxRetriesPerRequest plus retryStrategy.

[nicholasf]

For the record, we are just using the the Bluebird Promise timeout. Not great to mix promises with callbacks but it's the simplest.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

[walling]

I'm wondering if this issue should be re-opened, since I don't see the final solution implemented. This is something we could use too. Anyone care to explain or provide a code snippet for the idea behind flushing the offlineQueue in the retryStrategy?

Right now I'm researching different strategies to harden our codebase against various failures, fx. Redis becomes unresponsible. Imho I like the timeoutPerRequest option the most, even though it might be difficult to support efficiently. If it's a global option (ie. all commands have the same timeout value) it should be possible to implement an efficient strategy.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

[mjh1]

@luin would setting the autoResendUnfulfilledCommands or enableOfflineQueue option to false achieve the same result as flushing the offline queue inside the retryStrategy?

I'm curious to know exactly what was meant by "flushing the offlineQueue in the retryStrategy", I couldn't find example code in this thread, so maybe I'm missing something.

[elyobo]

Did anything ever happen with this @luin?

It seems like wanting the connection to generally keep retrying connections forever, but not wanting a specific request to block forever would be a common requirement. For example, I'm using redis as a to store state for a rate limiter, I don't want an individual request to get blocked forever so I don't want unbounded retries, but I do want the connection to be tried on the next request. It seems like the suggested maxRetriesPerRequest would do this (I can leave the retryStrategy unbounded, but limit the maximum retries on any given request).

I don't understand how the offline queue has any bearing on this behaviour; whether requests are queued up or not doesn't matter, I just don't want a specific request to keep being retried but I do want the connection to start retrying again when the next request is made.

[elyobo]

🎉 thanks @luin

[edevil]

I've read the comments to this issue but I haven't found a way to deal with unresponsive servers other than the promise timeout. When we are using third party modules that just take a redis connection as parameter (koajs/ratelimit for example) we can't use this method.

It seems unlikely that I will be able to persuade all the module authors to alter the way they access the redis server to include a timeout, it would be much easier if ioredis could take a statement timeout as a config parameter and enforce this.

[cvlahakis]

I stumbled on this thread today when looking into GET commands hanging due to cluster unavailability. Setting enableOfflineQueue to false produced the behavior I was looking for (fail fast).

[silverwind]

I'm also looking to get per-command timeouts implemented while keeping retryStrategy active. I tried using enableOfflineQueue but I observed ioredis unable to recover from a temporary connection failure so it does not seem like a option I could use.

Guess the only way to achieve this currently is to wrap all the command functions.

[ioredis-robot]

🎉 This issue has been resolved in version 4.25.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀