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

Decouple ActionCable application logic from low-level logic #27648

Closed
wants to merge 4 commits into from

Conversation

@palkan
Copy link
Contributor

@palkan palkan commented Jan 11, 2017

This PR is a continuation of the following conversation:

Me:

But, as for me, we should think about decoupling application logic from low-level logic. There are some places in the codebase, where we mix application level logic with socket level logic – the Connection class, for example. It would be great to extract its public API (that we use in ApplicationCable::Connection).

Another example is the explicit usage of the server's event loop in channels.

@dhh

I'd welcome refactorings to tease out the socket dynamics from the higher level bits. So do submit a PR on that.

So, here is it)

What has been done:

  • Connection has been split up into Connection (socket wrapper, low-level) and Client (although I don't like the name, app-level).
  • Channels code doesn't depend on server/sockets implementation, it only knows about an abstract client .

Client itself still depends on server and its event loop, but also can be isolated.

What's this refactoring for?

This will allow us to completely separate ActionCable public API from low-level sockets manipulations and thus will make it possible to use, for example, different server implementations with ease.

It will also simplify ActionCable channels unit-testing (#27191) and self-testing too.

P.S. "Refactor ActionCable streaming" PR (#27044) can be a part of this process.

@rafaelfranca rafaelfranca added this to the 5.1.0 milestone Jan 11, 2017
@palkan palkan force-pushed the palkan:refactor-action-cable branch Jan 11, 2017
Copy link
Member

@maclover7 maclover7 left a comment

I love the idea behind this PR, and I think this is a really, really good start at separating out some of the concerns with ActionCable::Connection. Besides for one concern, this PR looks amazing!

My fear is that we are introducing too many top-level classes for users to keep track of. If we merge in this PR, then we'll have three: Connection, Client, and Channel. IMHO, Client should be "internal" / "low-level / close to the metal" API, and should not be public for users to be interacting with.

It's possible we might not need to generate Connection anymore based on this PR, but that also brings up the issue of backwards compatibility with Rails 5 apps that already have #connect methods (just as an example) declared...

Thank you again so much for working on this, @palkan!!

actioncable/lib/action_cable/server/configuration.rb Outdated
@@ -11,7 +11,7 @@ class Configuration
def initialize
@log_tags = []

@connection_class = -> { ActionCable::Connection::Base }
@connection_class = -> { ActionCable::Client::Base }

This comment has been minimized.

@maclover7

maclover7 Jan 11, 2017
Member

Should we keep connection_class around, and introduce a new config option called client_class or similar? Otherwise in ActionCable::Server::Base#call we are declaring an explicit dependency on ActionCable::Connection::Base.

actioncable/test/client/authorization_test.rb Outdated
@@ -5,18 +5,20 @@ class ActionCable::Connection::AuthorizationTest < ActionCable::TestCase
class Connection < ActionCable::Connection::Base
attr_reader :websocket

def connect

This comment has been minimized.

@maclover7

maclover7 Jan 11, 2017
Member

Is there a way to keep this type of code inside of ActionCable::Connection::Base? Otherwise, when, for example, generating a new Rails application we'll have to create three ApplicationCable files: Connection, Client, and Channel...

@palkan
Copy link
Contributor Author

@palkan palkan commented Jan 12, 2017

@maclover7

My fear is that we are introducing too many top-level classes for users to keep track of. If we merge in this PR, then we'll have three: Connection, Client, and Channel. IMHO, Client should be "internal" / "low-level / close to the metal" API, and should not be public for users to be interacting with.

Let me clarify: this PR replaces Connection (which is internal) with Client. Which is definitely a bad idea(

We need a better name for "internal" entity and use "Connection" for the "public" one (Client).

What about "Socket"? Connection remains the same from the API point of view, Socket is a layer between raw WebSocket and public Connection.

@maclover7
Copy link
Member

@maclover7 maclover7 commented Jan 12, 2017

@palkan totally agree. We need to tease out a "public" facing Connection for users to continue to interact with, and have an internal Client / Socket (name obviously isn't super important) class where we hold "raw" / "metal" WebSocket-related details.

@palkan palkan force-pushed the palkan:refactor-action-cable branch 2 times, most recently Jan 14, 2017
@palkan
Copy link
Contributor Author

@palkan palkan commented Jan 14, 2017

@maclover7

We need to tease out a "public" facing Connection for users to continue to interact with, and have an internal Client / Socket (name obviously isn't super important) class where we hold "raw" / "metal" WebSocket-related details.

Done! Now it's backward-compatible.

@snood1205
Copy link

@snood1205 snood1205 commented Feb 22, 2017

Is full backwards compatibility ensured?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 23, 2017

@matthewd I'm deferring this to after 5.1. If you want to include it, let me know.

@rafaelfranca rafaelfranca removed this from the 5.1.0 milestone Feb 23, 2017
@palkan palkan force-pushed the palkan:refactor-action-cable branch Feb 25, 2017
@palkan
Copy link
Contributor Author

@palkan palkan commented Feb 25, 2017

Is full backwards compatibility ensured?

Integration tests are passed (without any change).

I've also checked my existent apps (e.g. this one I use for Action Cable experiments) – everything works fine.

@palkan palkan force-pushed the palkan:refactor-action-cable branch 4 times, most recently Feb 25, 2017
@palkan palkan force-pushed the palkan:refactor-action-cable branch to d1c330d Feb 26, 2017
@aried3r
Copy link
Contributor

@aried3r aried3r commented Aug 20, 2018

I just saw that there is some work ongoing for ActionCable regarding testing for Rails 6. I was wondering if some of the other ActionCable PRs by you are also to be revisited for Rails 6?

@palkan
Copy link
Contributor Author

@palkan palkan commented Aug 20, 2018

I was wondering if some of the other ActionCable PRs by you are also to be revisited for Rails 6?

I would be glad to revisit them too; and propose even more PRs later) I think, major release is a great chance to revisit the whole framework, to discuss its weak or missing parts and fix them.

@rails-bot
Copy link

@rails-bot rails-bot bot commented Dec 18, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 18, 2019
@rails-bot rails-bot bot closed this Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.