introduce start_async

[mikz]

• introduce run_async in celluloid/eventmachine
• introduce start_async in realtime client

This is useful for mixing slack client code with a web server.
Slack client itself is running asynchronously, but the web server is blocking.

Closes #28

Example:

client = Slack::RealTime::Client.new

socket = client.start_async

puts "still processing some work"

socket.disconnect!

[dblock]

What if we made start! take an options hash instead? And that would support things like async: true or whatever else is proprietary per driver?

[mikz]

I'd prefer smaller/more methods that accept less parameters/options. I like when code crashes on mistakes (like typos) instead of doing the wrong thing. If you mistype a method, it will crash. If you mistype an option, nothing might happen. I'd be ok with async: true if it would be a keyword argument #27 that crashes when mistyped.

Also I prefer two methods, because it means less conditionals in the code. If option is passed, something has do decide what to do for true/false. When when it is a method, it does just one thing, and always the same thing. Here the decision lies on the one who is using the client and there are no conditionals in the code.

[dblock]

I think we should take this opportunity to make an interface we want then. We want start and start_async and run and run_async? Want to make these consistent, upgrade README and UPGRADING as well? Also needs tests around these.

[dblock]

With this change, does it still make sense to have a separate Concurrency::Faye?

[mikz]

@dblock will think about it. I quite liked the bang because it says there is something dangerous (like never returning). I'd be in for making start_sync and start_async and have start! as an alias for now.

The run_* are not exposed, as they are only on the Socket classes, so there it does not matter much.
I'll add documentation to places I touch, to signal it is public/private api.

I think Concurrency::Faye still makes sense, because it has to handle starting EM reactor and start the Faye::WebSocket::Client.

Imho there is quite a lot of confusion in the naming (client, socket, driver, socket, ....) so at some point I'd like to rename them all to be more explicit what they do.

[dblock]

Makes sense. I think I just don't like that _async. Could be a start without the bang? Or is it too weird? Would you fix the build please and write some tests for these new calls, I'll merge.

Lets talk about renaming things, I agree that it's quite confusing. We should do it sooner than later because none of this stuff has been released yet.

[dblock]

Bump @mikz. Need help?

[mikz]

@dblock was traveling for last week so I had no chance to take a look. I might find some time this week, but would give it 50 % chance.

To continue the discussion. I think the version without bang would work, but it is too close to the one with bang and has so different semantics. If there would be one with and without bang I'd expect it to have raise exception / reconnect semantics instead of async/blocking behaviour.

In the future I think it should be start_blocking and start_async or similar to really show the intention.
ActionMailer moved to same semantics a while ago (deliver_now/deliver_later vs deliver/deliver_later).

So to reach some kind of decision I propose:

• keep start! as an alias to start_blocking (and maybe deprecate it in the future?)
• start_async as non blocking alternative that starts in background and returns an object that responds to #join that behaves like Thread#join
• start_blocking that keeps semantics of current start! but under the hood uses start_async.join

The naming does not have to be exactly like that, but I'd really suggest to have two methods like start_how.

Then there would be other option, to have just one start method but we would have Async drivers.
So every driver would have just one method, but you'd have to change the driver to be async version via the concurrency option like Slack::RealTimeClient.new(concurrency: Slack::RealTime::Concurrency::AsyncCelluloid). The blocking drivers would extend those async ones to introduce synchronisation.

I'm more inclined to to two methods instead of two drivers, but I'm not sure why. Imho it is less cognitive overhead to reason about.

[dblock]

I really like the multiple driver version, it uses inheritance instead of method naming and reduces the size of the API, which I think is a lot better. I think you like the initial proposal because of where the code evolved from, but if you step back and think about it less methods = less cognitive overhead and you don't have to think of this as a matrix. Thoughts?

[mikz]

@dblock I try to prefer composition over inheritance, so I'm not so thrilled about letting inheritance solve everything. Actually, the sync version will be more duplicated code.

Lets get to the code:

class BaseSocket
end

class AsyncCelluloidSocket < BaseSocket
  def start
     async.run
  end
end

class SyncCelluloidSocket < AsyncCelluloidSocket
  def start
    super.join
  end
end

class AsyncFaye < BaseSocket
  def start
    run_async
  end
end

class SyncFaye < AsyncFaye
  def start
    super.join
  end
end

So the super.join is duplicated across Sync implementations. Where if there would be two methods, it can be defined in the base.

class BaseSocket
  def start_blocking
    start_async.join
  end
end

class CelluloidSocket < BaseSocket
  def start_async
     async.run
  end
end

class Faye < BaseSocket
  def start_async
    run_async
  end
end

Imho, you'd also break Liskov substition principle when start would be always blocking method instead of returning something.

[dblock]

Fair enough. Bring this PR to a place where you want it merged and I will probably say "yes" :)

[mikz]

@dblock pushed work in progress, not so happy with it as i'm finding more stuff that should be somewhere else. Will give it few refactoring treatments and see. First candidates to move are the Socket#connect, Socket#connect and Client#run_loop.

[dblock]

Should go into the :development group.

[mikz]

yeah, didn't even want to commit it, got there by mistake

[dblock]

I think Gemfile.local might work for you for this kind of stuff.

[dblock]

I like this. Mostly I think it needs some tests around async. Would you be able to do that soon? I'd like to cut a release with these and related changes that have been piling up.

[dblock]

Note that the build is failing with You passed :install_if as an option for gem 'faye-websocket', but it is invalid. which probably means something :)

[mikz]

Yeah, it requires bundler 1.10 to work. How soon? I might have some time on Sunday, but not much before that. Next week should be much better.

[dblock]

Figured it all out, my problem with the integration test was that if you're going to do an async loop you don't have a blocking thread, so you need a different way to wait for the server (via a queue) or terminate the sync loop via an explicit kill (eg. EM.start). I chose the former.

Closing in favor of https://github.com/dblock/slack-ruby-client/pull/34