socketry / async-redis

MIT License
83 stars 18 forks source link

When redis-server is down, NotImplementedError is raised #10

Closed jeremyjung closed 5 years ago

jeremyjung commented 5 years ago

When redis-server goes down and a command is sent or client was waiting for a blocking pop, a NotImplementedError is raised.

Example stack trace:

        6: from /home/name/.gem/ruby/2.6.1/gems/async-1.15.2/lib/async/task.rb:199:in `block in make_fiber'
        5: from queue.rb:10:in `block in <main>'
        4: from /home/name/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/methods/keys.rb:37:in `exists'
        3: from /home/name/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/client.rb:104:in `call'
        2: from /home/name/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/pool.rb:51:in `acquire'
        1: from /home/name/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/client.rb:107:in `block in call'
/home/name/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/protocol/resp.rb:119:in `read_object': Implementation for token  missing (NotImplementedError)

Sample program to reproduce:

Async do
  client = Async::Redis::Client.new(Async::Redis.local_endpoint)
  _, command = client.blpop(10, "test")
end.wait
  1. Open redis-server
  2. Run sample application
  3. Close redis-server during blpop.

Ideally an error related to connection should be raised. For instance, a library specific CannotConnectError (Used in redis-rb gem) or lower level ECONNREFUSED

ioquatix commented 5 years ago

This should be something like EOFError if it occurs during middle of reading response.

ioquatix commented 5 years ago

This is where it should be failing I guess.

https://github.com/socketry/async-redis/blob/301852c6fac456524001ecc627ff3e35c2eb079f/lib/async/redis/protocol/resp.rb#L71

Let me take a closer look.

ioquatix commented 5 years ago

I think this is already fixed on master branch but there was one other case where it was possible so I also fixed that. Can you try master?

ioquatix commented 5 years ago

Sorry, I didn't push master on my other computer.... I did it just now.

jeremyjung commented 5 years ago

I switched the Gemfile to use master

Using async 1.15.4 from https://github.com/socketry/async (at master@d07dc4a)
Using async-io 1.19.0 from https://github.com/socketry/async-io (at master@4dfd149)
Using async-redis 0.3.3 from https://github.com/socketry/async-redis (at master@b0cd6db)

I still get the NotImplementedError when testing a get command while server is down and having the server disconnect while inside a blpop call.

jeremyjung commented 5 years ago

Actually my bad, it does raise a Async::Redis::ServerError. I think things are working.

jeremyjung commented 5 years ago

Sorry again, haha.. it does still have the NotImplementedError. The ServerError was a different issue. So I still see the same problem on master.

ioquatix commented 5 years ago

Can you give me stack trace from master

jeremyjung commented 5 years ago
Traceback (most recent call last):
    6: from /Users/jjung/.gem/ruby/2.6.1/gems/async-1.15.4/lib/async/task.rb:199:in `block in make_fiber'
    5: from redis.rb:7:in `block in <main>'
    4: from /Users/jjung/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/methods/lists.rb:27:in `blpop'
    3: from /Users/jjung/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/client.rb:104:in `call'
    2: from /Users/jjung/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/pool.rb:51:in `acquire'
    1: from /Users/jjung/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/client.rb:107:in `block in call'
/Users/jjung/.gem/ruby/2.6.1/gems/async-redis-0.3.3/lib/async/redis/protocol/resp.rb:119:in `read_object': Implementation for token  missing (NotImplementedError)
jeremyjung commented 5 years ago

My apologies again. If I clone the async-redis repository and create a sample program at base then require_relative './lib/async/redis' I get the expected error (EOFError).

I thought that having a separate project and pulling all the gems directly from master in the Gemfile would have the same result but it did not. I guess maybe I am missing something.

If you would like I can try to create a spec that disconnects the client and looks for an EOFError to return.

ioquatix commented 5 years ago

Sounds like a great idea

ioquatix commented 5 years ago

I thought that having a separate project and pulling all the gems directly from master in the Gemfile would have the same result but it did not. I guess maybe I am missing something.

Maybe try bundle update? Did it check out the latest revision from git? b0cd6dbad8dd589ed7665ce95e576471d1c7edaa

jeremyjung commented 5 years ago

I forgot to run my app with bundle exec so it was not using the gems in my Gemfile 😓 .

Thanks for your patience. I've submitted a PR with an initial attempt at a spec.

olleolleolle commented 5 years ago

Hello, @jeremyjung and @ioquatix!

11 was merged, and the tests are passing. 🎉

Can this issue be closed, now?

ioquatix commented 5 years ago

Yeah, let's close it.