client.call should start reactor for console debugging workflows

[matti]

currently if you try to do what all the best programmers do - use repl and try things out until it works and then you copy/paste the code - you can't do it because:

[1] pry(main)> c = Async::Redis::Client.new(Async::IO::Endpoint.tcp(ENV['REDIS_HOST'], 6379))
=> #<Async::Redis::Client:0x000055f0a598b3b8
 @endpoint=#<Async::IO::HostEndpoint:0x000055f0a598b458 @options={}, @specification=["redis", 6379, nil, 1]>,
 @pool=#<Async::Redis::Pool:0x000055f0a598b340 @active=0, @available=#<Async::Notification:0x000055f0a598b2f0 @waiting=[]>, @constructor=#<Proc:0x000055f0a598b278@/usr/local/bundle/gems/async-redis-0.3.1/lib/async/redis/client.rb:109>, @limit=nil, @resources=[]>,
 @protocol=Async::Redis::Protocol::RESP>
[2] pry(main)> c.call "BLPOP", "SLEEP", 1
RuntimeError: No async task available!
from /usr/local/bundle/gems/async-1.12.0/lib/async/task.rb:161:in `current'

So what about changing this:

def call(*arguments)
  @pool.acquire do |connection|
    connection.write_request(arguments)
    return connection.read_response
  end
end

to:

def call(*arguments)
  Async.run do
    @pool.acquire do |connection|
      connection.write_request(arguments)
      return connection.read_response
    end
  end
end

or even:

def call(*arguments)
  done = Async::Notification.new
  Async.run do |t|
    response = nil
    @pool.acquire do |connection|
      connection.write_request(arguments)
      done.signal connection.read_response
    end
    return done.wait
  end
end

note: I didn't even monkeypatch that locally, but I think you get the idea?

Although https://github.com/socketry/async#asyncreactorrun says

The cost of using Async::Reactor.run is minimal for initialization/server setup, but is not ideal for per-connection tasks.

but I think it would be ideal for programmer happiness?

[matti]

For example I did something like this for my experimental nats.io async client: https://github.com/matti/async-nats/blob/master/lib/async/nats/client.rb#L128-L137

[ioquatix]

There is a performance/convenience trade off, but it also alters the semantics because the function call becomes asynchronous.

I agree, it should be more simple for users.

In celluloid, we made something like object.async.method vs object.method.

[ioquatix]

The benefit of using an embedded Async.run do ... end is that the caller doesn't need to know anything in order to use the API. It's best if it's implicit IMHO.

[huba]

def call(*arguments)
  Async.run do
    @pool.acquire do |connection|
      connection.write_request(arguments)
      return connection.read_response
    end
  end
end

That does not work because call just returns an Async::Task.

The suggestion with Async::Notification works, but it breaks the error propagation, right now we raise an "Async::Redis::ServerError" when we parse a RESP error. With this suggestion we end up with an Async::Stop being raised. Not the end of the world, there is probably some room for improvement in the way RESP errors are handled/propagated anyway.

[ioquatix]

I think it's far too fine grained to make a task for every invocation. As you say, the error handling is horrible. But it's also a relatively big perf overhead, and also there needs to be some level of synchronisation between subsequent commands for 99% of user code.

[huba]

also there needs to be some level of synchronisation between subsequent commands for 99% of user code.

Not just that, internal stuff as well. The Pub/Sub nested context would also need its commands to run synchronously.

[huba]

Maybe there is a wider issue here, is there really a convenient way to do asynchronous stuff in REPL in general?

[ioquatix]

Yes, start the repl in an async task is one option

[huba]

I guess that would do it.

[matti]

yeah so my first suggestion was a brainfart. but what about something like:

def call(*arguments)
  done = Async::Notification.new
  Async.run do |t|
    @pool.acquire do |connection|
      connection.write_request(arguments)
      done.signal connection.read_response
    end
    return done
  rescue Async::Redis::ServerError => ex
    # something?
  end
end

which would allow the following to work in REPL?

c = client.call(...).wait

[matti]

although I'm not sure why the client should return a notification when you almost always want to get the response so you end up .waiting anyway?

[matti]

@huba

"With this suggestion we end up with an Async::Stop being raised." - why? I tried to write a test for this and I just get the actual exception, not Async::Stop. That said, I have seen Async::Stop being raised, but I'm not able to write any code that raises Async::Stop here.

[ioquatix]

All tasks are automatically a notification with a response, aka a promise.

def call(*arguments)
  Async.run do |task|
    @pool.acquire do |connection|
      connection.write_request(arguments)
      connection.read_response
    end
  end.wait
end

[huba]

Alright so here is the commit. @ioquatix what do you make of this failure. It fails the same way on all supported ruby versions. I have no idea where the said iteration is.

I decided to do what all good programmers do, go into repl (namely pry) and hack at it until it works the way I expect it to. First I started pry without wrapping it in an async task, everything worked as expected, I couldn't mimic any of the failures in the unit tests. Then I started pry inside an async task:

...
[2] pry(main)> c = Async::Redis::Client.new
...
[3] pry(main)> c.call 'NOTACOMMAND'
Async::Redis::ServerError: ERR unknown command `NOTACOMMAND`, with args beginning with:
from /home/huba/code/async-redis/lib/async/redis/protocol/resp.rb:111:in `read_object'
[4] pry(main)> c.call 'GET', 'something'
=> nil
[5] pry(main)> c.call 'GET', 'something'
RuntimeError: can't add a new key into hash during iteration
from /usr/lib/ruby/2.5.0/set.rb:349:in `add'
[6] pry(main)> wtf?
Exception: RuntimeError: can't add a new key into hash during iteration
--
0: /usr/lib/ruby/2.5.0/set.rb:349:in `add'
1: /home/huba/.gem/gems/async-1.10.3/lib/async/node.rb:87:in `parent='
2: /home/huba/.gem/gems/async-1.10.3/lib/async/node.rb:36:in `initialize'
3: /home/huba/.gem/gems/async-1.10.3/lib/async/task.rb:61:in `initialize'
4: /home/huba/.gem/gems/async-1.10.3/lib/async/reactor.rb:95:in `new'
5: /home/huba/.gem/gems/async-1.10.3/lib/async/reactor.rb:95:in `async'
6: /home/huba/.gem/gems/async-1.10.3/lib/async/reactor.rb:49:in `run'
7: /home/huba/.gem/gems/async-1.10.3/lib/async.rb:28:in `run'
8: /home/huba/code/async-redis/lib/async/redis/client.rb:100:in `call'
9: (pry):5:in `block in <main>'

All this time the key something is set up to be a list with some strings in it in redis and not nil.

Anyway, at least I know it's not something to do with rspec.

[ioquatix]

We have fixed exception handling and have an example async shell session, e.g.

https://github.com/socketry/async-redis/blob/880aada7951be58592ba1066551cc7b6b784e4d1/Rakefile#L8-L18

[ioquatix]

I don't really have a good answer for this yet, except it made me think about introducing a low overhead equivalent to Async{...}.wait called Sync{...}. If you perform Sync{...} in an existing reactor, it's a no-op, but if you do it by itself it is equivalent to Async{...}.wait. I think that can make a lot of sense in many situations.

[matti]

sounds great!

[troex]

I'll add my hack here, I try to use async-redis as a drop-in replacement where redis-rb + connection_pool is used (e.g. sidekiq works like this), so I came up with next monkey patching which works fine in falcon and in console:

module Async
  module Redis
    class Client
      def with
        if Async::Task.current?
          yield(self)
        else
          Async { yield(self) }.wait
        end
      end
    end
  end
end

Yes you still have to call like client.with {|c| c.info } but it works, I think Sync{...} makes more sense. Another possible solution is to write wrapper class with method_missing which will detect current reactor or wrap new one, but that's only for debugging/console because method_missing is slow.

[ioquatix]

Another idea I've been toying with:

Async{binding.irb}

We could consider introducing a standard command for this?