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
Add module for Rails 5.0 ActionCable streams #192
Conversation
0b2590d
to
e1ea5a1
Compare
Hi Tinco, This is very cool! I have a few high level comments:
Let me know what your thoughts are. Have a good one, |
This is the reason why I made it have an optional options hash that gets passed into changes, I think include_initial really depends on the use case, though I expect include_intial would be most common so we could make it the default.
I've discussed this with someone of the RethinkDB team about a year back, I think Daniel Mewes, they said that having in the thousands of database connections should be no issue. It is required by the way to have a connection per connection, there's no other way to do a changefeed. The bigger problem is that I block a global_io_executor thread per connection I think. We need some kind of asynchronous socket to circumvent this, or really do use a thread per connection (outside the global_io_executor). It would be nice if we could do without depending on EventMachine. Celluloid has https://github.com/celluloid/nio4r, which is a nice and small library which might allow us to use evented sockets without depending on EM or the whole of Celluloid. I will read up on exactly what Rails does, it internally has to keep track of websockets, surely they use evented sockets as well for that.
You are right. This will allow us to send subscription_confirmation as soon as we get the :initializing message. I am a bit short on time at the moment, hopefully I'll get some time to get to the bottom of these concerns later this week. |
The options hash is good. Let's leave the rethinkdb default which is false.
I agree. I don't think it's a good idea to use a thread+connection per channel. The faye-websocket gem depends on EventMachine, so maybe it's not a big deal to depend on EventMachine.
Awesome! let me know how it goes 👍 Also, if you need to write some tests, run them with Thanks, |
I don't really get how that works. Doesn't the 'changes' function always fully block a rethinkdb connection? When I researched last year I think that is what I was told. |
Let me show you real quick: ssh 3BHZt0Awbd96nmqLzVbCsTMI4@ny2.tmate.io or https://tmate.io/t/3BHZt0Awbd96nmqLzVbCsTMI4 |
I guess I'll show you another time, but yes, |
Hey sorry, that was a very cool invitation, unfortunately I was at work knee-deep in customer relation stuff :( |
Alright, I found how you did it, this doc: https://rethinkdb.com/docs/async-connections/#ruby-with-eventmachine very cool, I'll dig into that some more so I can use that in my own rethinkdb wrapper as well. |
Yes, NoBrainer uses the EventMachine API internally with Fibers: https://github.com/nviennot/nobrainer/blob/master/lib/no_brainer/query_runner/em_driver.rb I mentioned that we should have an async API instead being tied to EventMachine: |
awesome stuff! |
Oh jeez I forgot all about this PR, I went on a holiday and it just slipped my mind. |
Very interesting! You are probably already aware, but in case it helps Rails ActionCable uses the following three libraries under the hood:
(perhaps they may come in handy here) |
Just posting a wip as a reminder for me. My heart isn't really in it, I really have some kind of a principle against EM so I guess that demotivated me a bit. Hopefully tomorrow I'll feel like implementing some specs. And since we nobrainer is a ORM we should probably deserialize the values, right? |
@tinco no worries. I think it's okay to wait until the rethinkdb driver offer a better async API. |
I made a PR to RethinkDB that implements #async_run, and I got it to work in NoBrainer, hopefully I'll have a nice patch tomorrow. The RethinkDB team has been a little slow in giving feedback, but I think we'll get something in. |
Alright I pushed what I have right now. @nviennot could you advise me on this commit? There's two things I do that are awkward: I check out a new connection from your ConnectionManager that I call The second thing is that I call |
Hi @tinco :) It makes me happy that you are working on this :) :) We'll get it working. I don't have much understanding with ActionCable internals and their API semantics. Can you explain a bit how it works? Does the user "controller" returns once it calls Let's worry about connections later :) The middleware stack is definitely something we want to integrate with, because it handles various things like dealing with creating database or tables on demand, reconnections, logging, run options, etc. The middleware stack is comparable to the Rack middleware stack, it's essentially a bunch of "around_filter" to speak in rails controller land. Each middleware implements a call() method, which is responsible to call the next middleware. |
It just adds the subscription, you could do more things if you like in the
It gets closed when either the application explicitly calls
Yes at the moment, it might be decoupled later though.
No, all eventmachine code has been eliminated from Rails 5, the dependency you see is only a dev-dependency, I suppose for their test suite. (according to this gemspec)
There's a great introduction here. The basic idea is very simple. Instead of a So as I write this I realize it might not be such a good idea to override |
I see. So the following is more of a brain dump. I'm just laying out my thoughts for what we do moving forward. Regarding EventMachine, I see this: https://github.com/rails/rails/blob/master/actioncable/lib/action_cable/channel/periodic_timers.rb#L66 -- it seems that there's an event loop running in the background given the name of Let's talk about the API that we can expose to the user. I don't think we should use The API should:
Let me know if all this make sense to you. Thank you :) |
Correct, the event loop class that is used by default is located here. There is a mode where you use Faye instead of direct websockets, and Faye depends on EventMachine, but that's not enabled by default.
Ok, I dispatched on the main event loop but it would be better to do it as they recommend and use the workers for that.
I like the idea of using Of course we can use a unique name like |
I implemented a small demonstration of my idea with regards to extending |
Hi everyone. I have an issue with streams in Rails 5.
The channel looks like this: class ChatListChannel < ApplicationCable::Channel
include NoBrainer::Streams
def subscribed
chats_list = message_threads_collection
.order_by(filtering_index: :desc)
.limit(LIST_LIMIT)
.with_index(:filtering_index)
stream_from chats_list, { include_initial: true }, ->(change) { transmit_thread(change) }
end
def unsubscribed
# Any cleanup needed when channel is unsubscribed
end
end |
The code by @tinco doesn't handle DB connection drops well (see my previous point 2. at #192 (comment)). As this is outside of NoBrainer codebase, may I suggest that you open an issue on the nobrainer-stream project? Btw, I'm happy to provide some help if NoBrainer's code needs adjustment to provide for NoBrainer-streams |
He already opened an issue here: tinco/nobrainer_streams#3 , I'll try and find some time to look at it. |
Cool, thank you :)) |
@tinco do you have an update on this? It's really cool! |
Sure! |
Hi,
I made this small module to make the new ActionCable feature of Rails 5.0 work super conveniently with NoBrainer. You can see an example of its use here:
https://github.com/tinco/rails-chat-game/blob/master/app/channels/room_channel.rb
I use DHH's weird server side rendering technique there, if you skip that the code could be as simple as:
Pretty neat right?
I have not tested it in your project yet, if you think this is a good addition I'll work on getting the tests right so you could merge.
Kind regards,
Tinco