search

slack-ruby / slack-ruby-client

A Ruby and command-line client for the Slack Web, Real Time Messaging and Event APIs.
MIT License
1.21k stars 216 forks source link

Async handler support for Slack::RealTime::Config and Client #486

Closed milestruecar closed 1 year ago

milestruecar commented 1 year ago

Addresses https://github.com/slack-ruby/slack-ruby-client/issues/485

dblock commented 1 year ago

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?

dblock commented 1 year ago

While you're at it, see if putting an Async.run in any event (on) handler works for you? I think I'd prefer that change, as this problem exists for any long running task as a response to a Slack message. It would address https://github.com/slack-ruby/slack-ruby-client/issues/237.

milestruecar commented 1 year ago

While you're at it, see if putting an Async.run in any event (on) handler works for you? I think I'd prefer that change, as this problem exists for any long running task as a response to a Slack message. It would address #237.

edit: nevermind that original question didn't make sense... do you mean adding Async.run to every on block in that file? or is there some lower level method i can wrap in Async.run to catch all those events?

edit again: i tried updating this https://github.com/slack-ruby/slack-ruby-client/blob/bf5bb4be3e03e9bd74d22942a61d5611f5c1d997/lib/slack/real_time/client.rb#L235-L240 to 

handlers.each do |handler|
  Async.run { store.instance_exec(data, self, &handler) }
end

and it looks like it's working for my extremely narrow use case. Is that what you were after?

dblock commented 1 year ago

to

handlers.each do |handler|
  Async.run { store.instance_exec(data, self, &handler) }
end

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?

dblock commented 1 year ago

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?

milestruecar commented 1 year ago

to

handlers.each do |handler|
  Async.run { store.instance_exec(data, self, &handler) }
end

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?

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

dblock commented 1 year ago

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 async_handlers and it could take one of :none , :all and :each?

milestruecar commented 1 year ago

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

milestruecar commented 1 year ago

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

milestruecar commented 1 year ago

you were asking about exceptions earlier, here's an example of one of those eof errors in an async task:

    1m     warn: Async::Task [oid=0x5d5c] [ec=0x5d70] [pid=8722] [2023-06-30 12:48:29 -0700]
               | Task may have ended with unhandled exception.
               |   Faraday::SSLError: SSL_connect returned=1 errno=0 peeraddr=54.70.179.16:443 state=error: unexpected eof while reading
               |   → [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/protocol.rb:46 in `connect_nonblock'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/protocol.rb:46 in `ssl_socket_connect'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/http.rb:1342 in `connect'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/http.rb:1248 in `do_start'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/http.rb:1237 in `start'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:112 in `request_with_wrapped_block'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:102 in `perform_request'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:66 in `block in call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/adapter.rb:45 in `connection'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:65 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/response/logger.rb:23 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/slack-ruby-client/lib/slack/web/faraday/response/wrap_error.rb:16 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/request/url_encoded.rb:25 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-multipart-1.0.4/lib/faraday/multipart/middleware.rb:28 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/rack_builder.rb:153 in `build_response'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/connection.rb:444 in `run_request'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/connection.rb:280 in `post'
               |     [local path]/slack-ruby-client/lib/slack/web/faraday/request.rb:25 in `request'
               |     [local path]/slack-ruby-client/lib/slack/web/faraday/request.rb:11 in `post'
               |     [local path]/slack-ruby-client/lib/slack/web/api/endpoints/conversations.rb:248 in `conversations_list'
               |     [local path]/slack-ruby-client/lib/slack/web/pagination/cursor.rb:25 in `block in each'
               |     [local path]/slack-ruby-client/lib/slack/web/pagination/cursor.rb:22 in `loop'
               |     [local path]/slack-ruby-client/lib/slack/web/pagination/cursor.rb:22 in `each'
               |     [local path]/slack-ruby-client/lib/slack/web/api/endpoints/conversations.rb:244 in `conversations_list'
               |     [local path]/slack-ruby-client/lib/slack/real_time/stores/store.rb:356 in `block in <class
               |     [local path]/slack-ruby-client/lib/slack/real_time/client.rb:240 in `instance_exec'
               |     [local path]/slack-ruby-client/lib/slack/real_time/client.rb:240 in `block (2 levels) in run_handlers'
               |     [local path]/slack-ruby-client/lib/slack/real_time/client.rb:239 in `each'
               |     [local path]/slack-ruby-client/lib/slack/real_time/client.rb:239 in `block in run_handlers'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-1.31.0/lib/async/task.rb:261 in `block in make_fiber'
               |   Caused by OpenSSL::SSL::SSLError: SSL_connect returned=1 errno=0 peeraddr=54.70.179.16:443 state=error: unexpected eof while reading
               |   → [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/protocol.rb:46 in `connect_nonblock'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/protocol.rb:46 in `ssl_socket_connect'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/http.rb:1342 in `connect'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/http.rb:1248 in `do_start'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/3.2.0/net/http.rb:1237 in `start'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:112 in `request_with_wrapped_block'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:102 in `perform_request'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:66 in `block in call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/adapter.rb:45 in `connection'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:65 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/response/logger.rb:23 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/slack-ruby-client/lib/slack/web/faraday/response/wrap_error.rb:16 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/middleware.rb:17 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/request/url_encoded.rb:25 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-multipart-1.0.4/lib/faraday/multipart/middleware.rb:28 in `call'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/rack_builder.rb:153 in `build_response'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/connection.rb:444 in `run_request'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/faraday-2.7.9/lib/faraday/connection.rb:280 in `post'
               |     [local path]/slack-ruby-client/lib/slack/web/faraday/request.rb:25 in `request'
               |     [local path]/slack-ruby-client/lib/slack/web/faraday/request.rb:11 in `post'
               |     [local path]/slack-ruby-client/lib/slack/web/api/endpoints/conversations.rb:248 in `conversations_list'
               |     [local path]/slack-ruby-client/lib/slack/web/pagination/cursor.rb:25 in `block in each'
               |     [local path]/slack-ruby-client/lib/slack/web/pagination/cursor.rb:22 in `loop'
               |     [local path]/slack-ruby-client/lib/slack/web/pagination/cursor.rb:22 in `each'
               |     [local path]/slack-ruby-client/lib/slack/web/api/endpoints/conversations.rb:244 in `conversations_list'
               |     [local path]/slack-ruby-client/lib/slack/real_time/stores/store.rb:356 in `block in <class
               |     [local path]/slack-ruby-client/lib/slack/real_time/client.rb:240 in `instance_exec'
               |     [local path]/slack-ruby-client/lib/slack/real_time/client.rb:240 in `block (2 levels) in run_handlers'
               |     [local path]/slack-ruby-client/lib/slack/real_time/client.rb:239 in `each'
               |     [local path]/slack-ruby-client/lib/slack/real_time/client.rb:239 in `block in run_handlers'
               |     [local path]/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-1.31.0/lib/async/task.rb:261 in `block in make_fiber'
dblock commented 1 year ago

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

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 commented 1 year ago

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

milestruecar commented 1 year ago

do i need to somehow skip that test if CONCURRENCY isn't set? it seems like async isn't available with concurrency=none

dblock commented 1 year ago

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 Slack::RealTime::Socket called run_async(&_block) that raises NotImplemented, then override it in Slack::RealTime::Concurrency::Async::Socket with Async.run ... or something similar to def start_async(client).

Then condition the test that actually runs async to the existence of the CONCURRENCY env.

milestruecar commented 1 year ago

I'm not sure how to effectively test this since the socket is mocked

      context 'when config#async_handlers is :all', if: ENV['CONCURRENCY'] == 'async-websocket' do
          before do
            client.async_handlers = :all
            allow(socket).to receive(:run_async)
          end

          after do
            client.async_handlers = :none
          end

          it 'returns an Async::Task' do
            expect(client.send(:run_handlers, 'example', {})).to be_a Async::Task
          end
        end

this just returns nil for run_async

i know i can explicitly give a return allow(socket).to receive(:run_async).and_return(Async::Task) or something like that but then i'm not actually testing that method

dblock commented 1 year ago

Looks like some silliness in requires after my PR.


LoadError:
  cannot load such file -- async/websocket
# ./lib/slack/real_time/concurrency/async.rb:2:in `require'
# ./lib/slack/real_time/concurrency/async.rb:2:in `<top (required)>'
# ./spec/slack/real_time/concurrency/clients/async_spec.rb:4:in `require'
# ./spec/slack/real_time/concurrency/clients/async_spec.rb:4:in `<top (required)>'

LMK if you need help finishing this!

milestruecar commented 1 year ago

Looks like some silliness in requires after my PR.


LoadError:
  cannot load such file -- async/websocket
# ./lib/slack/real_time/concurrency/async.rb:2:in `require'
# ./lib/slack/real_time/concurrency/async.rb:2:in `<top (required)>'
# ./spec/slack/real_time/concurrency/clients/async_spec.rb:4:in `require'
# ./spec/slack/real_time/concurrency/clients/async_spec.rb:4:in `<top (required)>'

LMK if you need help finishing this!

it's due to the new test file

RSpec.describe Slack::RealTime::Concurrency::Async::Socket, if: ENV['CONCURRENCY'] == 'async-websocket' do

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

dblock commented 1 year ago

This is because we use Slack::RealTime::Concurrency::Async::Socket, and it auto-resolves async, requires it, then fails because the async-websocket library is not included in the Gemfile when CONCURRENCY is not set. To fix we change that to a string (so it doesn't resolve the class name), and condition it similarly to the other tests that skip with different values of CONCURRENCY for consistently mostly.

https://github.com/milestruecar/slack-ruby-client/pull/2

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 5790230078

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/slack/real_time/client.rb 6 7 85.71%
lib/slack/real_time/socket.rb 1 2 50.0%
<!-- Total: 37 39 94.87% -->
Totals Coverage Status
Change from base Build 5771061999: 0.02%
Covered Lines: 4872
Relevant Lines: 5466
💛 - Coveralls
milestruecar commented 1 year ago

This is because we use Slack::RealTime::Concurrency::Async::Socket, and it auto-resolves async, requires it, then fails because the async-websocket library is not included in the Gemfile when CONCURRENCY is not set. To fix we change that to a string (so it doesn't resolve the class name), and condition it similarly to the other tests that skip with different values of CONCURRENCY for consistently mostly.

milestruecar#2

i... did not know you could just drop a string in there. obviously i only rspec at about a 3rd grade level. Thanks!

dblock commented 1 year ago

Merged, thank you!