-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
client.call should start reactor for console debugging workflows #3
Comments
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 |
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 |
The benefit of using an embedded |
That does not work because call just returns an The suggestion with |
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. |
Not just that, internal stuff as well. The Pub/Sub nested context would also need its commands to run synchronously. |
Maybe there is a wider issue here, is there really a convenient way to do asynchronous stuff in REPL in general? |
Yes, start the repl in an async task is one option |
I guess that would do it. |
yeah so my first suggestion was a brainfart. but what about something like:
which would allow the following to work in REPL?
|
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 |
"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 |
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 |
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:
All this time the key Anyway, at least I know it's not something to do with rspec. |
We have fixed exception handling and have an example async shell session, e.g. Lines 8 to 18 in 880aada
|
I don't really have a good answer for this yet, except it made me think about introducing a low overhead equivalent to |
sounds great! |
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 |
Another idea I've been toying with:
We could consider introducing a standard command for this? |
Fix LREM command
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:
So what about changing this:
to:
or even:
note: I didn't even monkeypatch that locally, but I think you get the idea?
Although https://github.com/socketry/async#asyncreactorrun says
but I think it would be ideal for programmer happiness?
The text was updated successfully, but these errors were encountered: