SGrondin / bottleneck

Job scheduler and rate limiter, supports Clustering
MIT License
1.83k stars 79 forks source link

Redis errors with ReplyError: ERR UNKOWN_CLIENT #212

Open wmr-trevor opened 1 year ago

wmr-trevor commented 1 year ago

There appears to be an issue with redis connections. They work fine for a time and then they seem to break.

It appears to me as if the error message that was used to determine if the client should be re-registered has changed. Instead of it only being UNKNOWN_CLIENT it is now ERR UNKOWN_CLIENT https://github.com/SGrondin/bottleneck/blob/b83528333ba4d27cf70b81cc2be12e09d7ff692f/src/RedisDatastore.coffee#L89

The error received. This error happens exactly every 5 seconds.

ReplyError: ERR UNKNOWN_CLIENT
    at parseError (/app/node_modules/redis-parser/lib/parser.js:179:12)
    at parseType (/app/node_modules/redis-parser/lib/parser.js:302:14) {
  command: 'EVALSHA',
  args: [
    '8bd3e064b718321194a8df49c69e48fd0ffb4dcd',
    8,
    'b_node-print-rate-limit_settings',
    'b_node-print-rate-limit_job_weights',
    'b_node-print-rate-limit_job_expirations',
    'b_node-print-rate-limit_job_clients',
    'b_node-print-rate-limit_client_running',
    'b_node-print-rate-limit_client_num_queued',
    'b_node-print-rate-limit_client_last_registered',
    'b_node-print-rate-limit_client_last_seen',
    1677246715764,
    'fbe2isbe9mc'
  ],
  code: 'ERR'
}

In my setup I am passing in the redis connection details directly and using the redis datastore option.

const rateLimiter = new Bottleneck({
    maxConcurrent: 2,
    minTime: 100,

    // set the limiter id
    id: "node-print-rate-limit",

    // setup the redis store
    datastore: "redis",
    clientOptions: getDetails(),
    timeout: 30 * 1000,
});
dobrynin commented 1 year ago

FYI I used patch-package to implement the above suggestion and it works for me, thanks!

Here is the diff that solved my problem:

diff --git a/node_modules/bottleneck/lib/RedisDatastore.js b/node_modules/bottleneck/lib/RedisDatastore.js
index dc5943e..c7b8fdb 100644
--- a/node_modules/bottleneck/lib/RedisDatastore.js
+++ b/node_modules/bottleneck/lib/RedisDatastore.js
@@ -181,7 +181,7 @@ RedisDatastore = class RedisDatastore {
               return _this3.runScript(name, args);
             });
           }
-        } else if (e.message === "UNKNOWN_CLIENT") {
+        } else if (e.message === "ERR UNKNOWN_CLIENT") {
           return _this3.runScript("register_client", [_this3.instance.queued()]).then(() => {
             return _this3.runScript(name, args);
           });
justinadkins commented 1 year ago

Did anyone discover why the prefix ERR was added here? I see a lua script here that has the UNKNOWN_CLIENT error. We just ran into this problem when we bumped to Redis 7.

wmr-trevor commented 1 year ago

@justinadkins I haven't had a lot of extra time to get to the bottom of it but,

I came across some commits in the redis repo that seems to suggest where the prepended ERR comes from. https://github.com/redis/redis/blame/e7f18432b8e9e1d1998d3a898006497e6c5e5a32/src/script_lua.c#L1583

The version 7 release notes also mention it as a Potentially Breaking Change for pull 10329. https://github.com/redis/redis/releases/tag/7.0-rc2 https://github.com/redis/redis/pull/10329

At this point I don't have a better option then to use patch-package to get around the issue.

justinadkins commented 1 year ago

Thank you for this lightning fast, detailed response @wmr-trevor 🙏. We are also going to use patch-package to resolve this for now.

Sergiiivzhenko commented 1 year ago

@wmr-trevor thanks for the detailed clarification, if anybody faces the same issue here is a working solution in my repo: https://github.com/Sergiiivzhenko/bottleneck

published npm package: https://www.npmjs.com/package/@sergiiivzhenko/bottleneck

OscarMulder commented 1 year ago

I was facing exactly this issue, thanks for the solutions provided! 👏

maselious commented 1 year ago

Similar compatibility issue including SETTINGS_KEY_NOT_FOUND

There is a package with both fixes