animir / node-rate-limiter-flexible

Atomic counters and rate limiting tools. Limit resource access at any scale.
ISC License
3.07k stars 162 forks source link

RateLimiterRedis: Block set with secDuration=0 is reset on a consume method call #210

Closed vworld closed 1 year ago

vworld commented 1 year ago

Hi Thanks for this great library, saves a lot of time.

I have been testing the library and have a few questions about how it works for the block api method.

When a key is blocked with secDuration=0 it correctly rejects the .consume call. However it resets the msBeforeNext to the original duration value. Once the duration has expired the key no longer rejects. Is this the expected behavior?

And if it is, what would be the best way to block the key indefinitely at runtime.

Here is a test case using ava.

import {RateLimiterRedis} from "rate-limiter-flexible";
import test from "ava";

test(`RateLimiter blocked will reject consume but will reset "msBeforeNext" to the original duration, hence points are reset after duration`, async t => {
    const points = 5;
    const durationMs = 1000;
    const rateLimiterRedis = new RateLimiterRedis({
        storeClient: redisClient,   //ioRedis client
        points     : points,
        duration   : durationMs / 1000,
        keyPrefix  : "TestBlocking"
    });
    let res, key1 = `keyBlockedRejectsAlways_${Date.now()}`;

    // Consume a point
    res = await rateLimiterRedis.consume(key1, 1);
    t.assert(res && res.remainingPoints === points - 1);

    // Block the key
    res = await rateLimiterRedis.block(key1, 0);
    t.assert(res.remainingPoints === 0 && res.msBeforeNext === -1 && res.consumedPoints === points + 1);
    try {
        res = await rateLimiterRedis.consume(key1, 1)
        t.fail(`Consume after block should have failed. ${JSON.stringify(res)}`);
    } catch (rateLimiterRes) {
        t.assert(rateLimiterRes.remainingPoints === 0 && rateLimiterRes.msBeforeNext === durationMs && rateLimiterRes.consumedPoints === points + 2,
            `Should have rejected with expected keys. ${JSON.stringify(rateLimiterRes)}`);
    }

    // sleep for durationMs
    await sleep(durationMs + 50);
    try {
        res = await rateLimiterRedis.consume(key1, 1);
        t.fail(`Consume after block should have failed. ${JSON.stringify(res)}`);
    } catch (e) {
        t.assert(e && e.remainingPoints === 0 && e.msBeforeNext === durationMs && res.consumedPoints === points + 3);
    }
});

and the sleep function is a plain old promise

function sleep(ms: number) {
    return new Promise((resolve) => {
        setTimeout(resolve, ms)
    })
}
animir commented 1 year ago

@vworld Hi, thank you!

Unfortunately, there is no way to achieve that with a single consume call — it overwrites the duration for the key, so it will expire based on default limiter option. You can call get(key) to check if key is blocked forever and if so, don't call consume.

vworld commented 1 year ago

@animir Thank you for the quick response.

I was thinking about using blacklist but the docs say Blacklisted keys are blocked on code level not in store/memory. So I assume this will not work in a distributed system, where if a block is set in one process/node/machine it is honored by all processes/node/machine. Is that accurate?

And if we use RedisRateLimiter to block a key, the same will be propagated to redis and the block will be available across all machines that use the same key and same redis server. Is that correct?

animir commented 1 year ago

@vworld Yes, array of blocked keys stored in blackList is accessible for the current single process only. You could store blocked keys in any store and check it with isBlackListed function a way suitable for you. It is more work to store list of blocked keys somewhere else in database, but you could easily use Redis for that.

As of RateLimiterRedis, yes. Blocked keys are blocked on the Redis server level making it blocked for any process in distributed environment.

animir commented 1 year ago

@vworld Just another idea in addition to the answer for the first question. To store list of blocked keys you could easily store it with another instance of RateLimiterFlexible with options {points: 1, duration: 0}. If key should be blocked you can use set method, then check it with get method inside isBlackListed function of RLWrapperBlackAndWhite. But it is still one more request to the store, which shouldn't any problem though.

vworld commented 1 year ago

@animir Sounds good - will check both of these. Thanks again for all the help and time.

vworld commented 1 year ago

Just a heads up - Just like you suggested, I ended up using get to check if the key is blocked before using consume. The assumption here is that once a key is blocked with secDuration=0 the key is blocked forever with msBeforeNext = -1 until we try to consume the key again.

So something like this works for me while maintaining availability of the block thorough out the distributed setup. This basically goes with the paradigm of my codes that expect an error whenever a key is blocked.

    /**
     * Consumes the specified amount of points for a key
     * @throws {RateLimiterRes} - msBeforeNext > -1 if not blocked and msBeforeNext = -1 if key is blocked
     * @param {string} ip
     * @param {number} pointsToConsume
     * @returns {Promise<void>}
     * @private
     */
    private async _attemptToConsumePoints(ip: string, pointsToConsume: number = 1){
        const rLKey = this._getLimiterKey(ip);

        // check if key is blocked
        const res = await this.rateLimiter.get(rLKey);
        if (res && res.msBeforeNext === -1) {
            throw res;
        }
        await this.rateLimiter.consume(rLKey, pointsToConsume);
    }

While I have not tested the assumptions fully but initial tests look promising.

@animir is it safe to assume a blocked key will always return msBeforeNext = -1 until we consume

animir commented 1 year ago

@vworld Yes, until you change number of points for the blocked key, it will be blocked.

vworld commented 1 year ago

Sounds awesome, thank you for the support.