Skip to content
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

Added support for async-websocket. #219

Merged
merged 10 commits into from Sep 7, 2018

Conversation

Projects
None yet
2 participants
@dblock
Copy link
Collaborator

dblock commented Aug 25, 2018

Closes #210.

Seems to work for the example, but integration tests not so much. They are green because we don't run integration tests with a slack token to avoid exposing it in PRs. So WIP.

@dblock dblock force-pushed the dblock:async branch 2 times, most recently from 63e15bd to b06244d Aug 25, 2018

@dblock dblock force-pushed the dblock:async branch from b06244d to 6e07782 Aug 25, 2018

@dblock dblock changed the title Added support for socketry/async. Added support for async-websocket. Aug 25, 2018

@ioquatix

This comment has been minimized.

Copy link
Collaborator

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 6e07782 Aug 25, 2018

@dblock If you think of any way I can improve the WebSocket interfaces, please don't hesitate to start a discussion or make a PR.

This comment has been minimized.

Copy link
Owner Author

dblock replied Aug 27, 2018

Absolutely. All three implementations here need some serious refactoring and clarifying. Right now just trying to repeat what the old interfaces did and see what breaks.

@ioquatix

This comment has been minimized.

Copy link
Collaborator

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 6e07782 Aug 25, 2018

What's this used for?

This comment has been minimized.

Copy link
Owner Author

dblock replied Aug 27, 2018

Sending arrays, but it's possible that this is actually not useful with slack at all, will look at it later, thanks for pointing it out.

This comment has been minimized.

Copy link
Collaborator

ioquatix replied Aug 27, 2018

I think you need to invoke driver.binary

@ioquatix

This comment has been minimized.

Copy link
Collaborator

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 6e07782 Aug 25, 2018

If you have a nested flow control, you can process the event here. Otherwise, if you need to feed it back out to another part of the system, I'd recommend using Async::Queue.

@ioquatix

This comment has been minimized.

Copy link
Collaborator

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 6e07782 Aug 25, 2018

Pretty important to avoid MITM attacks!

This comment has been minimized.

Copy link
Owner Author

dblock replied Aug 27, 2018

I was going to use your implementation in async-http, but that got cumbersome.

This comment has been minimized.

Copy link
Collaborator

ioquatix replied Aug 27, 2018

How can we make it easier? What was cumbersome?

This comment has been minimized.

Copy link
Owner Author

dblock replied Aug 27, 2018

I honestly don't remember the specifics, but for now I am just trying to make this implementation as similar as possible to celluloid/em to refactor/extract later.

@ioquatix

This comment has been minimized.

Copy link
Collaborator

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 6e07782 Aug 25, 2018

The default is false, and that will never change, so you don't need to specify it.

@ioquatix

This comment has been minimized.

Copy link
Collaborator

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 6e07782 Aug 25, 2018

Just FYI, I'd suggest that this should be endpoint = ... because it's not a socket until you actually try to connect to it.

This comment has been minimized.

Copy link
Owner Author

dblock replied Aug 27, 2018

Yep, copy-paste from celluloid or EM implementation.

@ioquatix

This comment has been minimized.

Copy link
Collaborator

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 6e07782 Aug 25, 2018

You should rename this to endpoint or build_endpoint.

@dblock

This comment has been minimized.

Copy link
Owner Author

dblock commented on spec/integration/integration_spec.rb in 6e07782 Aug 27, 2018

@ioquatix This is not important, but is there a way to do something similar to what we do for EventMachine to ensure that a reactor is running?

https://github.com/slack-ruby/slack-ruby-client/blob/master/lib/slack/real_time/concurrency/eventmachine.rb#L45

Mostly this avoids telling the developer to wrap their own code in a .run when they just want to write a dumb client.

This comment has been minimized.

Copy link
Collaborator

ioquatix replied Aug 27, 2018

There is a way you can do this but it’s probably easier if I just help with this PR. Can you give me commit access?

This comment has been minimized.

Copy link
Owner Author

dblock replied Aug 27, 2018

You can push to my fork now, but you can also just fork the parent repo and make PRs or PR into my branch, whatever.

Integration tests passing (with a SLACK_API_TOKEN and CONCURRENCY=async-websocket) would be really useful too :)

@ioquatix

This comment has been minimized.

Copy link
Collaborator Author

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 58c2980 Aug 27, 2018

@dblock Fixed this. But maybe it can be removed or specs added?

@ioquatix

This comment has been minimized.

Copy link
Collaborator Author

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 58c2980 Aug 27, 2018

@dblock For normalising behaviour, run the reactor on it's own thread.

@ioquatix

This comment has been minimized.

Copy link
Collaborator Author

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 58c2980 Aug 27, 2018

Here we just block the caller, processing events sequentially.

@ioquatix

This comment has been minimized.

Copy link
Collaborator Author

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 58c2980 Aug 27, 2018

@dblock Loop until the driver is closed...

@ioquatix

This comment has been minimized.

Copy link
Collaborator Author

ioquatix commented on lib/slack/real_time/concurrency/async.rb in 58c2980 Aug 27, 2018

@dblock renamed.

@ioquatix

This comment has been minimized.

Copy link
Collaborator Author

ioquatix commented on spec/integration/integration_spec.rb in 58c2980 Aug 27, 2018

@dblock Not necessary.

@ioquatix

This comment has been minimized.

Copy link
Collaborator Author

ioquatix commented on 58c2980 Aug 27, 2018

@dblock all integration tests and specs pass.

This comment has been minimized.

Copy link
Owner

dblock replied Aug 27, 2018

Thanks @ioquatix, amazing.

1/10 runs I see this:

  1) integration test client connected responds to message
     Got 1 failure and 1 other error:

     1.1) Failure/Error: expect(client.started?).to be true
          
            expected true
                 got nil
          # ./spec/integration/integration_spec.rb:128:in `block (3 levels) in <top (required)>'
          # ./lib/slack/real_time/client.rb:161:in `block in callback'
          # ./lib/slack/real_time/client.rb:160:in `each'
          # ./lib/slack/real_time/client.rb:160:in `callback'
          # ./lib/slack/real_time/client.rb:98:in `block (2 levels) in run_loop'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/websocket-driver-0.7.0/lib/websocket/driver/event_emitter.rb:39:in `block in emit'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/websocket-driver-0.7.0/lib/websocket/driver/event_emitter.rb:38:in `each'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/websocket-driver-0.7.0/lib/websocket/driver/event_emitter.rb:38:in `emit'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/websocket-driver-0.7.0/lib/websocket/driver/hybi.rb:255:in `shutdown'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/websocket-driver-0.7.0/lib/websocket/driver/hybi.rb:367:in `emit_frame'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/websocket-driver-0.7.0/lib/websocket/driver/hybi.rb:123:in `parse'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/websocket-driver-0.7.0/lib/websocket/driver/client.rb:63:in `parse'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/async-websocket-0.6.1/lib/async/websocket/connection.rb:63:in `next_event'
          # ./lib/slack/real_time/concurrency/async.rb:46:in `run_loop'
          # ./lib/slack/real_time/concurrency/async.rb:36:in `block in connect!'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/async-1.10.3/lib/async/task.rb:74:in `block in initialize'

     1.2) Failure/Error: raise ThreadError, 'queue empty' if @queue.empty?
          
          ThreadError:
            queue empty
          # ./spec/support/queue_with_timeout.rb:29:in `block in pop_with_timeout'
          # ./spec/support/queue_with_timeout.rb:25:in `synchronize'
          # ./spec/support/queue_with_timeout.rb:25:in `pop_with_timeout'
          # ./spec/integration/integration_spec.rb:65:in `wait_for_server'
          # ./spec/integration/integration_spec.rb:76:in `block (2 levels) in <top (required)>'
          # ./spec/integration/integration_spec.rb:6:in `block (3 levels) in <top (required)>'
          # /Users/dblock/.rvm/gems/ruby-2.5.1/gems/vcr-3.0.3/lib/vcr.rb:244:in `turned_off'
          # ./spec/integration/integration_spec.rb:6:in `block (2 levels) in <top (required)>'

This is similar to a problem we see in EM on Travis occasionally. Maybe you'll spot something obvious.

This comment has been minimized.

Copy link
Collaborator Author

ioquatix replied Aug 27, 2018

It looks like a race condition. I will investigate.

This comment has been minimized.

Copy link
Collaborator Author

ioquatix replied Aug 27, 2018

Do you mind telling me what OS you are testing on to get that error? What ruby version?

This comment has been minimized.

Copy link
Collaborator Author

ioquatix replied Aug 27, 2018

Okay, managed to repro locally, 1/20 for me.

This comment has been minimized.

Copy link
Collaborator Author

ioquatix replied Aug 28, 2018

@dblock I believe this is, or is related to, slack-ruby#198

end
end

class Socket < Slack::RealTime::Socket
attr_reader :client

def start_async(client)
@client = client
client.run_loop
Thread.new do

This comment has been minimized.

@dblock

dblock Aug 27, 2018

Author Collaborator

We generally want the caller to be responsible for calling this, otherwise we're spawing a thread per client (or a bot)? Or this this necessary even if you run the code within a Async::Reactor.run?

This comment has been minimized.

@ioquatix

ioquatix Aug 27, 2018

Contributor

I did it to make it the same as how Celluloid is working, which spawns one thread per actor AFAIK. However, it's not necessary. We can push that requirement further up the call chain, but it might mean that the top level needs to embed the code in a reactor. Let me try to figure out the race conditions first then we can figure out how to push that code further up.

@dblock dblock force-pushed the dblock:async branch from db4851f to 78b64ed Aug 27, 2018

@ioquatix

This comment has been minimized.

Copy link
Contributor

ioquatix commented Aug 27, 2018

@dblock What's your preference for how it behaves?

If you want to make a server with multiple connections/clients, can we make something to wrap around that? so that we can have a top level reactor, or enforce event machine reactor creation?

Threading makes everything tricky and on Ruby you will have worse performance except in very specific situations. So if we can avoid it, it will make code simpler.

@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Aug 27, 2018

@ioquatix This is generally used in a stack of libraries, for example

https://github.com/slack-ruby/slack-shellbot
uses https://github.com/slack-ruby/slack-ruby-bot-server
uses https://github.com/slack-ruby/slack-ruby-bot
uses slack-ruby-client

So we tell users to put reactor type stuff as high as possible. For example slack-ruby-bot-server is the first opinionated library that currently defaults to Celluloid and can/should default to async in the next big release.

So for this library we should be removing any ::Async::Reactor.run except for tests. But maybe that doesn't hurt at all? It looks reentrant, we can also keep it?

I removed one in connect! that didn't seem necessary. But it looks like I need one per thread so I have to keep the one in start_async.

I also fixed the example and I think this PR if green is ready to go. I'll wait for the dust to settle and to hear from you ;)

@ioquatix

This comment has been minimized.

Copy link
Contributor

ioquatix commented on d3140e9 Aug 27, 2018

Haha, good idea.

@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Aug 27, 2018

Thanks so much for your help @ioquatix. If you want to play with a real bot, slack-shellbot is a fun one. You'll have to replace references to Celluloid in https://github.com/slack-ruby/slack-ruby-bot-server.

@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Aug 27, 2018

Async branch for slack-ruby-bot in slack-ruby/slack-ruby-bot#198.

@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Aug 27, 2018

Async branch for slack-ruby-bot-server in slack-ruby/slack-ruby-bot-server#75

@ioquatix How does one do timers in async correctly? What's the equivalent of this:

class Foobar
  include Celluloid

  def whatever
    every 10 do
     # timers
    end
  end
end
@ioquatix

This comment has been minimized.

Copy link
Contributor

ioquatix commented Aug 27, 2018

Async actually uses the same timers and nio4r gems as celluloid, so some parts are the same. That being said, you are better using an explicit loop.

require 'async/reactor'

Async::Reactor.run do |task|
	while true
		task.sleep(2)
		puts "Hello World"
	end
end
@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Aug 27, 2018

slack-ruby/slack-shellbot#11, and https://shell.playplay.io/ is running with this, lets see how it behaves over the next few days

@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Aug 28, 2018

@ioquatix

  • What's the equivalent of this with async? How do I force-close a connection from the outside?

  • How do I terminate a task from within? The equivalent of Celluloid terminate?

That ping thread is still noticing disconnects, so I want to put the ping thread back to working and then we can debug what's going on.

@ioquatix

This comment has been minimized.

Copy link
Contributor

ioquatix commented Aug 28, 2018

  • What's the equivalent of this with async? How do I force-close a connection from the outside?

The following is a thread-safe way to stop a reactor:

thread = Thread.new do
  @reactor = Async::Reactor.new

  @reactor.run(&block)
end

@reactor.stop
thread.join

It's safe to call from anywhere.

With respect to your specific question, you probably don't want to kill the reactor unless you can guarantee you created it at the very top level (otherwise you will end up potentially stopping other unrelated tasks. In this specific case, you are fine to do that since you are creating it at the top of a new thread.

If you weren't doing that, you need to capture the task, and call Task#stop, which will terminate that task and all sub-tasks, e.g.

def start_async(client, task: ::Async::Task.current)
	task.async do
		client.run_loop
	end
end

# Elsewhere:
task = start_async(client)

# To stop:
task.stop
@ioquatix

This comment has been minimized.

Copy link
Contributor

ioquatix commented Aug 28, 2018

In theory, this also works:

def start_async(client)
	::Async::Reactor.run do
		client.run_loop
	end
end

# Elsewhere:
task = start_async(client)

# To stop:
task.stop # This might actually be the reactor if none was created, because the operation blocked, calling stop is a no-op, but it's still valid to do it.
@ioquatix

This comment has been minimized.

Copy link
Contributor

ioquatix commented Aug 28, 2018

How does the life-cycle of Slack::RealTime::Socket work?

Firstly, it's a bit confusing since it's not a socket, is it?

Then, it has several methods:

      def connect!
        # I've modified this a bit in my local code...
        unless connected?
          connect
          logger.debug("#{self.class}##{__method__}") { driver.class }
        end
        
        yield driver if block_given?
      end

      def disconnect!
        driver.close
      end

      def connected?
        !driver.nil?
      end

      def start_sync(client)
        thread = start_async(client)
        thread.join if thread
      rescue Interrupt
        thread.exit if thread
      end

      def start_async(_client)
        raise NotImplementedError, "Expected #{self.class} to implement #{__method__}."
      end

      def close
        @driver = nil
      end

I can sort of see why we have connect and connect! although the distinction is a bit confusing, and calling connect multiple times seems like a bug. I don't understand why we have close and disconnect!.

Where exactly should we call @reactor.stop?

@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Aug 28, 2018

You can see slack-ruby/slack-ruby-bot-server#75 for the changes I made to the ping thread, seems to work OK.

@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Aug 28, 2018

You're right that Slack::RealTime::Socket is not a socket, it's a connection. It's probably an artifact of the past because it represents a websocket.

It can exist in two ways, synchronously and asynchronously, so there's a bit of confusion between the two "modes" I think. You either start_sync or start_async and you can connect or disconnect it at any time for whatever reason (possibly not even used), then finally cleanup by calling close. Or at least that's the intent. We can probably get rid of close.

I am not sure why or where we want to stop the reactor explicitly, do we?

@dblock dblock referenced this pull request Aug 28, 2018

Closed

Slack-side disconnects #208

@dblock dblock force-pushed the dblock:async branch 5 times, most recently from e6f4509 to 6df313c Sep 3, 2018

@dblock dblock force-pushed the dblock:async branch 4 times, most recently from 0250cd0 to 694b574 Sep 6, 2018

@dblock dblock force-pushed the dblock:async branch from 694b574 to 115942e Sep 7, 2018

@dblock

This comment has been minimized.

Copy link
Collaborator Author

dblock commented Sep 7, 2018

I'm merging this as is.

@dblock dblock merged commit adf9672 into slack-ruby:master Sep 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dblock dblock deleted the dblock:async branch Sep 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.