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

Error: "No matching script. Please use EVAL." #124

Open
zarqman opened this issue Feb 11, 2023 · 11 comments
Open

Error: "No matching script. Please use EVAL." #124

zarqman opened this issue Feb 11, 2023 · 11 comments

Comments

@zarqman
Copy link

zarqman commented Feb 11, 2023

It seems that redlock v2.0.0 is possibly broken when used with redis 4.8.1 (the redis gem, not redis-server).

When attempting to lock, I'm getting "Redis::CommandError: NOSCRIPT No matching script. Please use EVAL."

Full exception
Redis::CommandError: NOSCRIPT No matching script. Please use EVAL.
redis (4.8.1) lib/redis/client.rb:162:in `call'
redis (4.8.1) lib/redis.rb:270:in `block in send_command'
redis (4.8.1) lib/redis.rb:269:in `synchronize'
redis (4.8.1) lib/redis.rb:269:in `send_command'
redis (4.8.1) lib/redis/commands.rb:206:in `call'
redlock (2.0.0) lib/redlock/client.rb:171:in `block in lock'
redlock (2.0.0) lib/redlock/client.rb:210:in `recover_from_script_flush'
redlock (2.0.0) lib/redlock/client.rb:170:in `lock'
redlock (2.0.0) lib/redlock/client.rb:260:in `block (2 levels) in lock_instances'
redlock (2.0.0) lib/redlock/client.rb:260:in `select'
redlock (2.0.0) lib/redlock/client.rb:260:in `block in lock_instances'
redlock (2.0.0) lib/redlock/client.rb:313:in `timed'
redlock (2.0.0) lib/redlock/client.rb:259:in `lock_instances'
redlock (2.0.0) lib/redlock/client.rb:234:in `block in try_lock_instances'
redlock (2.0.0) lib/redlock/client.rb:230:in `times'
redlock (2.0.0) lib/redlock/client.rb:230:in `try_lock_instances'

Does redlock need to require 'redis', '>= 5.0.0' ?
(If so, redlock 2.0.0 might need to be yanked to avoid breaking apps currently using redis gem < 5.)

Using redlock 1.3.2 works correctly.

Versions

redis-server 6.2.10
redlock 2.0.0
redis (gem) 4.8.1
rails 7.0.4.2

@jarkko
Copy link

jarkko commented Feb 15, 2023

@zarqman We have this same issue with 2.0.0 and 2.0.1 even though we're using redis 5.0.6.

    redis (5.0.6)
      redis-client (>= 0.9.0)
    redlock (2.0.1)

The code in question (works fine with 1.3.2):

      lock_manager.lock!(access_token_lock_key(type), 1000) do
        REDIS.with { |redis|
          redis.mapped_hmset(access_token_key(type), {
                               token:,
                               expires_at: expires_at.to_s
                             })
        }
      end

…now causes:

     Redis::CommandError:
       NOSCRIPT No matching script. Please use EVAL.
     # ./app/lib/saxo_open_api/stored_access_token.rb:28:in `set'
     # ./spec/lib/saxo_open_api/api_client_spec.rb:152:in `block (5 levels) in <top (required)>'

@ujh
Copy link

ujh commented Feb 15, 2023

Which version of Redis (the database, not the gem) are you using, @jarkko? I saw that error, before upgrading to at least v6.

@jarkko
Copy link

jarkko commented Feb 15, 2023

Which version of Redis (the database, not the gem) are you using, @jarkko? I saw that error, before upgrading to at least v6.

6.2.5

@jarkko
Copy link

jarkko commented Feb 15, 2023

FWIW, bumping to 7.0.8 made no difference.

I guess the issue is somehow related to the switch from Redis to RedisClient in Redlock. Our own client above (REDIS) is still a Redis instance.

@ujh
Copy link

ujh commented Feb 15, 2023

Yes, that's the other thing I had to change. You cannot use anything like Redis.new anymore, you need to pass a RedisClient instance.

@leandromoreira
Copy link
Owner

I tried to warn about this fact in the readme, let me know what you guys think about it?

@ujh
Copy link

ujh commented Feb 15, 2023

I am not sure there's much more you can do. Maybe you could add it to the 2.0.0 release description? Then dependabot will most likely pick it up and people will directly see it.

You could try catching that error and reraise it with a message that mentions that it requires Redis 6, but that seems pretty brittle and might now be worth it.

@zarqman
Copy link
Author

zarqman commented Feb 15, 2023

@leandromoreira, the updated readme is definitely a help! 👏

I'd suggest a couple of additional things:

  1. Don't refer to a "redis" version anywhere, as it's unclear whether it's the server or gem. Always qualify it as "redis-server" or "redis gem" (which should possibly be "redis-client gem" anyway now--but maybe not, depending on the next item).

  2. Is a Redis::Client instance (from the redis gem) allowed? It's a subclass of RedisClient (from the redis-client gem), so perhaps? Then, if Redis::Client is allowed, can it be safely pulled / reused from Redis#client (ie: Redis.new(...).client)?

    If it's possible to use Redis::Client and/or Redis#client, then it'd be nice to just use them so everything works as-is.

    On the other hand, if either (or both) of those don't work, then raise an exception in Redlock::Client#initialize (or Redlock::RedisInstance#initialize) explaining what is and isn't allowed. This would raise before getting to any potential "No matching script" error, and would be much clearer indication to a dev as to what needs to be changed for Redlock v2 to work properly.

  3. If either Redis::Client and/or Redis#client don't work, then it might be useful, in the readme, to call those out specifically as not valid.

For additional background, the apps I work on have historically reused an existing redis (gem) client instance (instantiated with Redis.new) with Redlock. This was to reduce the total number of connections to Redis because some Redis hosting services limit the allowed connections. If Redlock can't safely share a client anymore, no problem, but that historical way of doing things might be why this is tripping some of us up. 😄

I appreciate everyone's effort in making Redlock the best as it can be. Thanks for your generosity!

@jarkko
Copy link

jarkko commented Feb 16, 2023

One simple addition to @zarqman's notes would be to add a changelog file. It would be trivial to spot the changes from there , whereas you don't necessarily read through the Readme every time you bump the version of a gem. Dependabot can also automatically show the changes from a changelog.

In our context, we're using some more advanced functionality of the redis gem that the pure redis-client does not support, so switching the client would be a bit of a refactoring. In any case, looking at the changes between 1.3.2 and 2.0, there doesn't seem to be much we would benefit from anyway, so that's fine.

Anyway, thanks for your hard work on Redlock. Much appreciated!

jarmopi added a commit to puavo-org/puavo-web that referenced this issue Mar 6, 2023
Unfortunately, some Redis-related gems have to be locked. There's
something funky going on with Redlock:

leandromoreira/redlock-rb#124

Gem updating is already stressfull enough without this kind of
garbage. And when something related to Redis breaks, I take no
chances. I'll play it safe and lock all Redis-related gems. I
will unlock them one by one and fix errors. But that will come
later.
@bibendi
Copy link

bibendi commented Jun 8, 2023

Hi there.

It seems, if we want to reuse an existing Redis gem

REDIS = ConnectionPool::Wrapper.new do
  Redis.new(url: ENV['REDIS_URL'])
end

and get a backward compatibility, we should catch all the errors here

rescue RedisClient::CommandError => e
and just check for NOSCRIPT in the message because with the Redis gem we get Redis::CommandError, not the RedisClient::CommandError

@jeffbax
Copy link

jeffbax commented Jan 24, 2024

Just ran into this upgrading to the latest activejob-uniqueness gem (which brought along a new Redlock) https://github.com/veeqo/activejob-uniqueness

Basically, I found I had to do what this guy was saying: #135 (comment)

It wasn't totally clear to me reading https://github.com/leandromoreira/redlock-rb#redis-client-configuration

Redlock::Client expects URLs, or configurations or Redis objects on initialization. Redis objects should be used for configuring the connection in more detail, i.e. setting username and password.

That I was no longer fine passing in Redis.new objects into the config, when it now seems to require RedisClient.new instances instead.

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

6 participants