redis-rb / redis-client

Simple low level client for Redis 6+
MIT License
124 stars 60 forks source link

Provide minimal Ractor compatibility #163

Closed jgmontoya closed 8 months ago

jgmontoya commented 8 months ago

Addresses https://github.com/redis-rb/redis-client/issues/162 while keeping the ability to accept subclasses of accepted types as command arguments (https://github.com/redis-rb/redis-client/commit/9fb715fb1a3e1caf91d9b7fa921532085d47c707).

The tradeoff made is losing the cache on the subclass. I think this is reasonable as the length of DUMP_TYPES.keys is fixed to 4.

jgmontoya commented 8 months ago

@byroot I'm having issues getting the tests to work within the test framework 😅

For some reason Ractor.new fails and I'm not understanding why. I'd appreciate a hand with this if you have any idea of what could be going on.

I tried creating a simple test: ractor_test.rb

# frozen_string_literal: true

require "test_helper"

class RactorTest < Minitest::Test
  include ClientTestHelper
  def test_set_within_ractor
    Ractor.new do
      within_ractor_redis = new_client
      within_ractor_redis.call("SET", "foo", "bar")
    end

    out_of_ractor_redis = new_client
    assert_equal("bar", out_of_ractor_redis.call("GET", "foo"))
  end

  def test_get_within_ractor
    out_of_ractor_redis = new_client
    out_of_ractor_redis.call("SET", "foo", "bar")

    ractor = Ractor.new do
      within_ractor_redis = new_client
      within_ractor_redis.call("GET", "foo")
    end

    assert_equal("bar", ractor.take)
  end
end

I did however manage to get it to work "manually" (i.e. outside of the testing framework) in a separate script (had to rename some imports from require to require_relative to make sure it was taking the local project):

# frozen_string_literal: true

require_relative 'lib/redis_client'

def new_client
  RedisClient.new(host: 'localhost', port: 53285, timeout: 0.1)  # I had a redis instance running on that port
end

def test_get_within_ractor
  out_of_ractor_redis = new_client
  out_of_ractor_redis.call('SET', 'foo', 'bar')

  ractor = Ractor.new do
    within_ractor_redis = new_client
    within_ractor_redis.call('GET', 'foo')
  end

  p ractor.take == 'bar' # => true
end

def test_set_within_ractor
  Ractor.new do
    within_ractor_redis = new_client
    within_ractor_redis.call('SET', 'foo', 'bar')
  end

  out_of_ractor_redis = new_client
  p out_of_ractor_redis.call('GET', 'foo') == 'bar' # => true
end

def test_get_set_within_ractor
  ractor = Ractor.new do
    within_ractor_redis = new_client
    within_ractor_redis.call('SET', 'foo', 'bar')
    within_ractor_redis.call('GET', 'foo')
  end

  p ractor.take == 'bar' # => true
end

def test_multiple_ractors
  ractor1 = Ractor.new do
    within_ractor_redis = new_client
    within_ractor_redis.call('SET', 'foo', 'bar')
    within_ractor_redis.call('GET', 'foo')
  end

  p ractor1.take == 'bar' # => true

  ractor2 = Ractor.new do
    within_ractor_redis = new_client
    within_ractor_redis.call('GET', 'foo')
  end

  p ractor2.take == 'bar' # => true
end

test_get_within_ractor
test_set_within_ractor
test_get_set_within_ractor
test_multiple_ractors

Any ideas on how we could take that "manual testing" unto minitest?

byroot commented 8 months ago

For some reason Ractor.new fails and I'm not understanding why.

Protip: never ever write in a bug report or ticket that something "fails" without describing what that failure look like, generally that implies sharing the error message and / or backtrace.

jgmontoya commented 8 months ago

For some reason Ractor.new fails and I'm not understanding why.

Protip: never ever write in a bug report or ticket that something "fails" without describing what that failure look like, generally that implies sharing the error message and / or backtrace.

You're absolutely right, sorry about that!

Here it is with full trace:

❯ bundle exec rake test:ruby TEST=./test/redis_client/ractor_test.rb --trace
** Invoke test:ruby (first_time)
** Execute test:ruby
starting toxiproxy... started with pid=60025
starting redis... started with pid=60026
Waiting for toxiproxy (port 8474)... ready.
Waiting for redis (port 16380)... ready.
Running test suite with driver: RedisClient::RubyConnection
Run options: --seed 18910

# Running:

/Users/j/Documents/redis-client/test/redis_client/ractor_test.rb:21: warning: Ractor is experimental, and the behavior may change in future versions of Ruby! Also there are many implementation issues.
E/Users/j/Documents/redis-client/test/redis_client/ractor_test.rb:8: warning: Ractor is experimental, and the behavior may change in future versions of Ruby! Also there are many implementation issues.
E

Finished in 0.004395s, 455.0627 runs/s, 0.0000 assertions/s.

  1) Error:
RactorTest#test_get_within_ractor:
RuntimeError: /Users/j/Documents/redis-client/test/redis_client/ractor_test.rb:21: warning: Ractor is experimental, and the behavior may change in future versions of Ruby! Also there are many implementation issues.

    test/support/raise_warnings.rb:7:in `warn'
    <internal:warning>:51:in `warn'
    <internal:ractor>:277:in `new'
    test/redis_client/ractor_test.rb:21:in `test_get_within_ractor'

  2) Error:
RactorTest#test_set_within_ractor:
RuntimeError: /Users/j/Documents/redis-client/test/redis_client/ractor_test.rb:8: warning: Ractor is experimental, and the behavior may change in future versions of Ruby! Also there are many implementation issues.

    test/support/raise_warnings.rb:7:in `warn'
    <internal:warning>:51:in `warn'
    <internal:ractor>:277:in `new'
    test/redis_client/ractor_test.rb:8:in `test_set_within_ractor'

2 runs, 0 assertions, 0 failures, 2 errors, 0 skips
rake aborted!
Command failed with status (1): [ruby -w -I"lib:test:lib" /Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/rake_test_loader.rb "./test/redis_client/ractor_test.rb" ]
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/testtask.rb:130:in `block (3 levels) in define'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/file_utils.rb:57:in `sh'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/file_utils.rb:102:in `ruby'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/testtask.rb:117:in `block (2 levels) in define'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/file_utils_ext.rb:58:in `verbose'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/testtask.rb:111:in `block in define'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/task.rb:281:in `block in execute'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/task.rb:281:in `each'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/task.rb:281:in `execute'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/task.rb:219:in `block in invoke_with_call_chain'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/task.rb:199:in `synchronize'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/task.rb:199:in `invoke_with_call_chain'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/task.rb:188:in `invoke'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:182:in `invoke_task'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:138:in `block (2 levels) in top_level'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:138:in `each'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:138:in `block in top_level'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:147:in `run_with_threads'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:132:in `top_level'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:83:in `block in run'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:208:in `standard_exception_handling'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/lib/rake/application.rb:80:in `run'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rake-13.1.0/exe/rake:27:in `<top (required)>'
/Users/j/.rbenv/versions/3.3/bin/rake:25:in `load'
/Users/j/.rbenv/versions/3.3/bin/rake:25:in `<top (required)>'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/cli/exec.rb:58:in `load'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/cli/exec.rb:58:in `kernel_load'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/cli/exec.rb:23:in `run'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/cli.rb:483:in `exec'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/cli.rb:31:in `dispatch'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/cli.rb:25:in `start'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/exe/bundle:48:in `block in <top (required)>'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/lib/bundler/friendly_errors.rb:103:in `with_friendly_errors'
/Users/j/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/bundler-2.3.13/exe/bundle:36:in `<top (required)>'
/Users/j/.rbenv/versions/3.3/bin/bundle:25:in `load'
/Users/j/.rbenv/versions/3.3/bin/bundle:25:in `<main>'
Tasks: TOP => test:ruby

Let me know if you want me to push all of this (failing test and "passing" manual test along with the require => require_relative changes) to the branch btw

byroot commented 8 months ago

Right, the backtrace points you exactly at what is going on: https://github.com/redis-rb/redis-client/blob/84f1c6567eb02a941e94e8ca41f97d55a026ec4c/test/support/raise_warnings.rb#L7

Ractors will print a warning when you use them because they are still experimental, and I have something that turn warnings into errors in the test suite.

Just add something like return if message.include?("Ractor is experimental") before the raise

jgmontoya commented 8 months ago

Right, the backtrace points you exactly at what is going on:

https://github.com/redis-rb/redis-client/blob/84f1c6567eb02a941e94e8ca41f97d55a026ec4c/test/support/raise_warnings.rb#L7

Ractors will print a warning when you use them because they are still experimental, and I have something that turn warnings into errors in the test suite.

Just add something like return if message.include?("Ractor is experimental") before the raise

Right, the backtrace points you exactly at what is going on:

https://github.com/redis-rb/redis-client/blob/84f1c6567eb02a941e94e8ca41f97d55a026ec4c/test/support/raise_warnings.rb#L7

Ractors will print a warning when you use them because they are still experimental, and I have something that turn warnings into errors in the test suite.

Just add something like return if message.include?("Ractor is experimental") before the raise

OMG can't believe I missed that, thank you!

I've just uploaded some basic tests, let me know if I'm missing something or if you'd like me to test anything else with more depth

jgmontoya commented 8 months ago

@byroot fixed flaky test, skipped them altogether for ruby versions lower than 3 and made them compatible with ruby 3.0 (https://bugs.ruby-lang.org/issues/17592). Also added a small description on the unreleased section of the changelog.

casperisfine commented 8 months ago

Alright, I refactored your PR quite a bit, and the Ractors in 3.0 won't make it because RedisClient.default_driver. But should work for basic operations on 3.1+.