firehoseio / firehose

Build realtime Ruby web applications. Created by the fine folks at Poll Everywhere.
http://firehose.io/
MIT License
726 stars 72 forks source link

Batch messages in server to emit over WebSockets and HTTP Long Polling transports #53

Closed bradgessler closed 8 years ago

bradgessler commented 8 years ago

When a client connects over WebSockets or HTTP, Firehose connects to Redis once per message. This is inefficient if there's more than one message that should be replayed to the client. The Firehose server should grab as many messages as it can from Redis, store in memory, and replay to the client with one connection.

bradgessler commented 8 years ago

I started working on this at https://github.com/firehoseio/firehose/tree/redis-efficiency.

@bakkdoor I think this is going to solve the problem in https://github.com/firehoseio/firehose/issues/51 if we do this right. Also, if you take a look at this branch I think we can move a lot of the message checking logic out of the exec callback (https://github.com/firehoseio/firehose/blob/redis-efficiency/lib/firehose/server/channel.rb#L38-L60) and into the MessageSequence class that will be much easier to unit test and audit.

Feel free to branch from this and move forward when the sun rises in Germany.

bakkdoor commented 8 years ago

I branched off into redis-efficiency-cb and added some fixes

bradgessler commented 8 years ago

I pulled your changes into my branch and added a lot more specs for MessageOffset. The entire suite currently has 10 failing tests. If I get the offset specs passing (running into negative array errors, easy to fix but I need to run) then the others should start passing.

Test suite is at https://github.com/firehoseio/firehose/blob/redis-efficiency/spec/lib/server/message_offset_spec.rb. Another set of eyes on that would be good for a sanity check to make sure the failing tests are correct.

I think we could get rid of the MessageOffset#subscribable? method and simply use MessageOffset#empty? (as a delegation to messages) when we get the MessageOffset suite passing.

bradgessler commented 8 years ago

I moved this branch to https://github.com/firehoseio/firehose/tree/message-buffer. There's not a class called MessageBuffer with very explicit tests around the sequence and messages_list that we store in Redis. This should help us track down when nil messages are being emitted from Firehose. Once we have this solid base we can work forward into the transports then integration tests.

There's currently 3 failures in the integration tests that I suspect may be bugs. Still need to dive into this code and figure out what's going on.

bradgessler commented 8 years ago

This code was merged into master and deployed in 1.3.5.