Async handler support for Slack::RealTime::Config and Client #486
Conversation
|
LGTM, needs fixing tests, CHANGELOG, and maybe we can refactor the implementation a bit into private methods so that it's a bit more digestible? |
|
While you're at it, see if putting an |
edit: nevermind that original question didn't make sense... do you mean adding edit again: i tried updating this slack-ruby-client/lib/slack/real_time/client.rb Lines 235 to 240 in bf5bb4b and it looks like it's working for my extremely narrow use case. Is that what you were after? |
Right that's what I am after. Should it wrap each handler or all handlers? Is there a change in exception handling when one handler fails with the wrapped code? |
CHANGELOG.md
Outdated
| @@ -11,6 +11,7 @@ | |||
| * [#475](https://github.com/slack-ruby-client/pulls/475): Update API from [slack-api-ref@977dad5](https://github.com/slack-ruby/slack-api-ref/commit/977dad5) - [@slack-ruby-ci-bot](https://github.com/apps/slack-ruby-ci-bot). | |||
| * [#476](https://github.com/slack-ruby-client/pulls/476): Update API from [slack-api-ref@d0b2989](https://github.com/slack-ruby/slack-api-ref/commit/d0b2989) - [@slack-ruby-ci-bot](https://github.com/apps/slack-ruby-ci-bot). | |||
| * [#478](https://github.com/slack-ruby-client/pulls/478): Update API from [slack-api-ref@d797055](https://github.com/slack-ruby/slack-api-ref/commit/d797055) - [@slack-ruby-ci-bot](https://github.com/apps/slack-ruby-ci-bot). | |||
| * [#486](https://github.com/slack-ruby/slack-ruby-client/pull/486): Async cache build for slack::realtime::stores::store - [@milestruecar](https://github.com/milestruecar). | |||
There was a problem hiding this comment.
Capitalize and quote Slack::Realtime::... to match actual class name.
|
If this "just works", then I'd like a spec that ensures that handlers are run inside an async block, and we should talk about backwards compatibility and something in the README that says that handlers execute async. Then, what could go wrong? :) Or maybe this should be optional behind a configuration option? |
i guess it depends on expected behavior, this makes it... more async? if order of handler execution matters, it would be better to have the entire block wrapped in a single async task. if order of execution doesn't matter (and everyone is fine spinning up some arbitrary number of async tasks) then this implementation seemed to work as expected for me |
|
Good point on order of execution. Current behavior is that it's predictable, so at a minimum we should keep the current behavior. If we bump the major version we can change the default. How about introducing an option? We could call it |
|
tried implementing an async_handlers option with none all and each but i can't tell if all and each are actually doing anything meaningfully different. everything seems to fire off in the same order, the only real difference I can see is the each option is throwing some uncaught eof errors that i'm having trouble chasing down while crawling through the conversations.list loop. current theory is that doing each handler async is running afoul of the retry logic, though i'm surprised i didn't see similar behavior with the code in this pr since it's the same |
|
just having :all as the default behavior makes sense to me. i can't picture a scenario in which someone would want to opt into :none and block the client from receiving from slack while the RealTime::Stores::Store cache gets hydrated, and i'm having trouble creating a scenario in which :each does anything meaningfully different |
|
you were asking about exceptions earlier, here's an example of one of those eof errors in an async task: |
It's backwards incompatible, and would require a major version bump. I'm just trying to ask to default to the current behavior. Btw, not everybody uses stores. I disable that in my bots. |
milestruecar
changed the title
|
there might be a cleaner way to implement this than a case statement. i tried a tricky metaprogramming thing first but it was pretty gross. idk what your preference is |
lib/slack/real_time/client.rb
Outdated
| @@ -234,13 +234,24 @@ def dispatch(event) | |||
|
|
|||
| def run_handlers(type, data) | |||
| handlers = store.class.events[type.to_s] | |||
| handlers.each do |handler| | |||
| store.instance_exec(data, self, &handler) | |||
| case @async_handlers | |||
There was a problem hiding this comment.
Use case async_handlers so it can be overwritten.
This reverts commit e4f877b.
|
do i need to somehow skip that test if CONCURRENCY isn't set? it seems like async isn't available with concurrency=none |
I see what's happening. This comes from the fact that we used to support different concurrency libraries. So the async handler option is something that may have different implementations depending on the async library. Rather than muting the test add a method in Then condition the test that actually runs async to the existence of the |
|
I'm not sure how to effectively test this since the socket is mocked this just returns i know i can explicitly give a return |
spec/slack/real_time/client_spec.rb
Outdated
| end | ||
|
|
||
| after do | ||
| client.store.class.events.merge!(@events) | ||
| client.async_handlers = :none |
There was a problem hiding this comment.
Store the original value of async_handlers.
spec/slack/real_time/client_spec.rb
Outdated
| @events = client.store.class.events.dup | ||
| client.store.class.events.clear | ||
| client.async_handlers = :all | ||
| allow(socket).to receive(:run_async).and_return(Async.run { nil }) |
There was a problem hiding this comment.
This just replaces run_async with an implementation that doesn't run anything or test that the correct thing was called. In an actual test you're interested in the different behavior with :all vs. :none, so it could look something like this:
For :all
it 'runs tasks async' do
expect(socket).to receive(:run_async)
client.send(:run_handlers, 'example', {}))
endFor :none
it 'runs tasks async' do
expect(socket).to_not receive(:run_async)
client.send(:run_handlers, 'example', {}))
endSimilarly you have a good test already to ensure that run_handlers returns an Async task.
it 'returns an Async::Task' do
expect(client.send(:run_handlers, 'example', {})).to be_a Async::Task
endThere should be the equivalent of that that does not return an Async::Task with :none.
Makes sense?
There was a problem hiding this comment.
that first and last points are what i was talking about in my last comment, it doesn't return an Async::Task out of the box because the socket is mocked. It just gets nil back
There was a problem hiding this comment.
when i run without that explicit return i get this
Failures:
1) Slack::RealTime::Client client with a full store #start! #run_handlers when config#async_handlers is :all returns an Async::Task
Failure/Error: expect(client.send(:run_handlers, 'example', {})).to be_a Async::Task
expected nil to be a kind of Async::Task
# ./spec/slack/real_time/client_spec.rb:182:in `block (6 levels) in <top (required)>'
There was a problem hiding this comment.
This was expected. In this test the socket client (the part that is actually async) is mocked rather than using the real concurrency class.
let(:ws) { double(Slack::RealTime::Concurrency::Mock::WebSocket, on: true) }It therefore never invokes the real Slack::RealTime::Concurrency::Async::Socket and so you can't check the return of that.
The test here is good, it checks that 1) when async_handlers=:all we call run_async on the socket class, and 2) when async_handlers=:none we don't call it. If we want to check that run_async works as expected for the real Slack::RealTime::Concurrency::Async::Socket we take it out into a separate spec for that, which I added in milestruecar#1. I think we need to add tests there (in a future PR), which reminds me to add code coverage ;)
|
Looks like some silliness in requires after my PR. LMK if you need help finishing this! |
it's due to the new test file i think it's trying to include that class regardless of ENV['CONCURRENCY'] on initialization, i'm not sure how to get around this, short of just fully excluding that file on rspec command line |
|
This is because we use |
Pull Request Test Coverage Report for Build 5790230078
💛 - Coveralls |
i... did not know you could just drop a string in there. obviously i only rspec at about a 3rd grade level. Thanks! |
|
Merged, thank you! |
Addresses #485