-
Notifications
You must be signed in to change notification settings - Fork 653
Redis as a replication backend for scalability #140
Conversation
1e2838f
to
2e4569e
Compare
@WyriHaximus if you have the time, I wouldn't mind some clarification on how to handle reconnection with lazy clients. I think we'd only need to re-subscribe to the channels on reconnect (i.e. |
85c522f
to
8b27984
Compare
Alright, I have the presence channel logic in. That was probably the hardest part to write for this feature. I don't love it, I'm hooking in all over the place to reach out to the replication backend, and had to change how some methods work such that they can return promises, but it looks like it should be functional (no pun intended). At this point, it's mostly ready for code review, I'll start writing tests next chance I get to work on this. Sometime next week probably. |
I'll try to get an answer about that tomorrow by doing some experiments because I'm already interested in that 👍 |
Awesome, thank you! |
2a53a12
to
bdc8b6c
Compare
I added tests to cover Replication. I went the route of extending some of the existing tests, only running the tests with replication enabled as well, to hit those relevant code paths. RedisClient is not covered, a FakeReplication mock is used instead. The implementation is looking pretty good at this point, I'll just need to do some actual end-to-end testing to confirm everything is working as expected, and I'm waiting to hear back from @WyriHaximus on reconnection (which is mostly just a nice-to-have, IMO not a requirement to merge). @mpociot or @freekmurze I'd really appreciate it if this could be reviewed soon, because #6 seemed pretty popular! Let me know if there's anything I can do to make this easier to review, I realize it's quite a big patch. I'm willing to chat about it out-of-band if it can help. |
Sorry it took a bit longer then expected. Based on some experiments and digging through the code. You only need to resubscribe and maybe re-do any operations that haven't completed yet. Be careful with this tho. Your state in redis might have changed compared to what you want to execute. Also keep an eye on the |
Sorry, I'm not sure I fully understand. Do you mean doing something like this should be fine? // Pseudocode
$conn->on('close', function($conn) use ($subscriptions) {
foreach ($subscriptions as $sub) {
$conn->subscribe($sub);
}
}); I don't see a way to explicitly reconnect for lazy clients. I'm not sure how I'd reconcile the close and error events, since those are separate things. |
There is no need to explicitly reconnect for lazy clients, that's build in. Here are my two experiment files. You can restart redis without issue, but note the timeout in the subscriber, that is required due to the redis server needed some time to restart and when we resubscribe right away we're our connection might be refused:
<?php
use Clue\React\Redis\Factory;
require 'vendor/autoload.php';
$loop = React\EventLoop\Factory::create();
$factory = new Factory($loop);
$client = $factory->createLazyClient('localhost');
$client->on('error', function (Throwable $throwable) {
echo (string)$throwable;
});
$loop->addPeriodicTimer(1, function () use ($client) {
$message = json_encode(['id' => 10, 'time' => time()]);
$client->publish('user', $message);
});
$loop->run();
<?php
use Clue\React\Redis\Factory;
require 'vendor/autoload.php';
$loop = React\EventLoop\Factory::create();
$factory = new Factory($loop);
$client = $factory->createLazyClient('localhost');
$client->on('message', function ($channel, $payload) {
// pubsub message received on given $channel
var_dump($channel, json_decode($payload));
});
$client->subscribe('user');
$client->on('unsubscribe', function () use ($client, $loop) {
echo '__unsubscribe__', PHP_EOL;
$loop->addTimer(1, function () use ($client) {
$client->subscribe('user');
});
});
$client->on('error', function (Throwable $throwable) {
echo (string)$throwable;
});
$loop->run(); |
Awesome, that's very helpful! Reading that along with looking at I was thinking, would there be a need for exponential backoff for connecting at the LazyClient level? Seems to me like it only tries to connect again one time in |
shrug I'm unsure tbh because this is blurring the line between providing a simple |
Fair enough. But I think that enough of the implementation of the connection is handled by the LazyClient class that I don't think it's really possible to implement that sort of thing without extending or changing that class. Anyways, thanks a bunch for your help! Very much appreciated. |
You could do a decorator or an adapter implementing that behaviour. But that might be something for a follow up PR tbh Any time, feel free to ping me when you need help with anything ReactPHP related things 😎 |
Alright I have good news - I finally got around to trying this branch out in my own project to test things out for-real. It works! I didn't try out all the features, because my app doesn't cover everything. I almost exclusively use private channels, no presence channels... so I'd need someone else to try this out with presence channels to confirm that those work as intended. I'll make a PR on the docs repo later, but here's a quick writeup to start. I figure this'll be added under the "Advanced usage" section. Configure
|
@francislavoie This looks great. I will give it a go in one of my apps this week. Great work! |
@francislavoie how about this trait? he was added only in 5.8 |
@phantom8805 you're mentioning that as a concern for compat with older Laravel versions? That's a good point, I'll take a look soon. I really just copied the source of the Pusher broadcaster and replaced the broadcast function with Redis instead. |
@francislavoie and thank for this pull request. it fixed my problem. |
this looks great.. I wish we would hear from @mpociot in this and other awesome PRs to get an idea of what he likes/doesn't like and if this is on its way to ever being merged |
I’ve already started reviewing and merging a couple of PRs in the last days. |
I'll say that from my part, at the very least I appreciate that you mentioned that this inspired your work rather than not giving any attribution - I sent you an email in case you want to chat about it. |
gonna throw a big +1 out there for this |
I assume this package is no longer maintained or something. A few days ago I was in an urge to fixing some Websocket issues so I had to re-create a package from scratch, using most of this codebase code: https://github.com/renoki-co/sock. The only thing I haven't replicated to the package is the dashboard, which is less of a concern right now. Why rewriting it? Most of the code wasn't commented, no code coverage to know what has been tested and what has not been tested, I have added more tests to cover some more code and use cases, and now trying to make it running well in a horizontally-scaled environment. It's 11 PM and I have been working since 8 AM on trying to make it scalable until I reached my end of not being able to serialize the connections when storing channels, and they were too big to store them, even more, when there are hundreds of connections. I have opened a PR here with the changes from contributor's PR: https://github.com/renoki-co/sock/pull/4 I will be able to bring up the feature testing with a Redis instance and test it with both Local and Redis instances to ensure they both work as stated in this PR. In case you're asking, I haven't covered up the documentation since the primary concern is to make it work. |
@rennokki I wouldn't be so quick to call this package abandonned but unfortunately this seems to be a trend with beyondcode/marcel's packages.. he's so busy making new things all the time that there isn't much time left for improving/maintaining his old packages.. I absolutely love his stuff and use almost everything he's put out and purchased every product beyondcode has sold so far.. but I do wish he hired help to maintain his old packages such as botman, this and others. This PR in particular should have been merged ages ago.. it's such a must-have for anyone that needs to work with bigger numbers. Anyway I appreciate your attempt to fix this problem and thank you for creating https://github.com/renoki-co/sock, I've starred it and am watching it, I hope you add some documentation and perhaps a migration guide for migrating from laravel-websockets or at least some docs to explain if this is also a pusher drop-in replacement and what similarities/differences it has with laravel-websockets |
Just to be clear - I'm not trying to get people to use it, I offer it as an alternative. I know how it feels when scaling things out to millions of requests everyday with improper resources. I honestly hope this package will get proper maintenance asap and look after issues/PRs. |
As maintainer of one of Marcel's smaller packages (https://github.com/mpociot/teamwork) I can really recommend being a maintainer to free him up, so he can do even more cool stuff 😍 |
@okaufmann @mpociot I'd want to offer as a maintainer too on packages that need support. 🤔 |
@rennokki I just sent you a message via Twitter DM, but as I said in a previous comment:
Just send me a Twitter DM (they are open) and we can talk. I just can't find the time to work on laravel websockets at the moment tbh Update: @rennokki is now added as a maintainer 🎉 |
For transparency, I spoke with @mpociot as well and I said no because I'm no longer actually using this library in an active project at my company, so I wouldn't really be able to dogfood. I don't think I'd be that effective as a maintainer. That said, @rennokki, please feel free to @ me for reviews/feedback if you want, I'm willing to help out in that way. I'm glad to see this PR and project finally see some movement after nearly two years with the original proposal for horizontal scaling being in #6. I hope it sees a proper revival. 📈 |
@francislavoie I changed the base branch from Looking forward to fixing the StyleCI pipeline checks in |
In laravel8 I have an error with redis-pusher for broadcast in env. |
@athamidn - So for horizontal scaling, did you just do the following?
and then edit websockets.php to add the following?
This weirdly does not work for me. Any idea why? Also, if you could point me to any documentation, that'll be helpful! |
@francislavoie - wondering if you still have any comments on this? The work done here is fabulous but unfortunately nowhere mentioned in any docs. Any pointers will be super helpful. |
I'm not involved in maintenance of this project anymore. Sorry. |
This is a continuation of @snellingio's work in #61 and supersedes it.
Disclaimer: this is still WIP, I still have some work to do here before it's ready to go.
Things that are done:
I fixed some typos here and there, added some additional type hints to make my IDE happy, added
@mixin
on Facades, etc.RedisClient
to use lazy clients (thanks @WyriHaximus for implementing that feature!) and implemented pub/sub.RedisClient
will ignore messages from itself.RedisPusherBroadcaster
is implemented.This is a hybrid of the Redis and Pusher broadcasters that are shipped with Laravel. This is needed because we still want to use the Pusher auth logic (signing the broadcasted messages) but we want to broadcast via Redis instead of doing an HTTP request to the websocket server to push out messages.
This is needed so that channels from different apps don't cross-talk when they aren't supposed to. This is done in the Broadcaster and RedisClient. Redis channels are names
"$appId:$channel"
wherever needed.This one was tricky, because among other things, it required rewriting some of the HTTP controller logic to support Redis' async IO. The replication feature flag complicates this a bit as well because we end up with two code paths throughout, wherever it's enabled. I'll probably need the most help reviewing this portion due to its complexity.
I went the route of extending some of the existing tests, only running the tests with replication enabled as well, to hit the relevant code paths.
RedisClient
is not covered, aLocalClient
mock is used instead.RedisChannelManager
) because it doesn't itself do anything.Channel
andPresenceChannel
are where the interesting things happen. Maybe these classes could be split up into replicated versions of each, but it doesn't seem entirely necessary yet.Things that are still to do:
In case Redis goes down,
RedisClient
should attempt to reconnect, and if successful, should re-subscribe to all the channels on behalf of the users. This shouldn't be too hard, there's already local storage for the list of channels (seeprotected $subscribedChannels
inRedisClient
)We'll need new sections in the documentation to describe how to set this up. Notably, users will need to add a new driver in
broadcasting.php
due to the hybrid broadcaster I implemented.