Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should lua script be reloaded into cache on NOSCRIPT failure? #1438

Closed
ebertz opened this issue Sep 29, 2021 · 3 comments
Closed

Should lua script be reloaded into cache on NOSCRIPT failure? #1438

ebertz opened this issue Sep 29, 2021 · 3 comments

Comments

@ebertz
Copy link

ebertz commented Sep 29, 2021

https://github.com/luin/ioredis/blob/62b6a648910eccc3d83a3acd2db873704fd2080a/lib/script.ts#L35-L51

When a Lua script is loaded using defineCommand(), ioredis will load the script into cache memory, and attempt to reference it using the SHA.

If the redis cluster is restarted or otherwise flushes the script cache, calling the custom script will first attempt to do an EVALSHA, then upon receiving a NOSCRIPT error, will call EVAL with the lua code stored in app memory.

Unless the application adds logic to reload the script, each subsequent call will always do 2 operations: a failed EVALSHA followed by an EVAL. This will obviously affect performance.

Instead of doing EVALSHA followed by EVAL each time the missing script is called, could ioredis instead handle NOSCRIPT errors by reloading the lua script into the redis script cache? That way, subsequent calls will be successful when trying the initial EVALSHA

@marcbachmann
Copy link
Collaborator

marcbachmann commented Jan 30, 2022

In the pipeline code scripts are handled differently. They get loaded once and not retried unless a client side interval expires: https://github.com/luin/ioredis/blob/master/lib/pipeline.ts#L376

How do you call the script exactly?
In my case EVAL doesn't even get executed.

The following workaround will work if maxScriptsCachingTime is set to a higher value, as it will force a script load on reconnect. But already queued commands are still rejected:

const redis = new Redis({
  maxScriptsCachingTime: 3600
})

redis.on('reconnecting', () => {
  redisClient._addedScriptHashes = {}
  log.debug({instance}, `Redis connection reconnecting`)
})

I guess we can just put that logic into the connect function: https://github.com/luin/ioredis/blob/b8177479c348aa4bbd467fa944d61fe9b35aec19/lib/redis/index.ts#L304-L322

     // Make sure only one timer is active at a time
     clearInterval(this._addedScriptHashesCleanInterval);
+    this._addedScriptHashes = {};

@marcbachmann
Copy link
Collaborator

marcbachmann commented Feb 1, 2022

Fixed in #1499
An EVAL always loads a script automatically into the cache. With the update, ioredis now tries to send evalsha on a second execution. This will result in much less network traffic if the scripts are bigger.

@marcbachmann
Copy link
Collaborator

marcbachmann commented Mar 14, 2022

Some more improvements landed on main and 5.0.0-beta.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants