Lighthouse: Subscriptions using redis rather than pusher

[thekonz]

What problem does this feature proposal attempt to solve?

Currently, the subscription logic is implemented with pusher in mind. That's pretty nice for the 80/20 default use case, but I wanted it to work locally with redis and laravel echo server.

That is why I made the Lighthouse Redis Broadcaster and an apollo link to go with it.

Which possible solutions should be considered?

1. Integrate the code of the package into lighthouse itself
2. Do not integrate it, but link to it in the docs

Any input is welcome, I am fine with either solution and would like to know how you are feeling about this. I firstly wanted to solve my own problem and I hope I can solve one for someone else.

[spawnia]

https://github.com/tlaverdure/laravel-echo-server seems to be reasonably popular, i will happily include a driver as a part of Lighthouse. We should not (soft-)lock users into using Pusher, adding additional options is a great idea.

Can your solution be added as another broadcaster in the configuration?

[thekonz]

Yes, it is adding itself as a broadcaster in the config and even using the routes part to set up the authentication hook.

[spawnia]

Nice. Looking forward to a PR to integrate this within Lighthouse.

Let's try and keep configuration minimal and contained to lighthouse.php. As long as it doesn't become to complex, it would be great if users only have to choose between the options there to configure subscriptions.

[thekonz]

@spawnia Hey, one question.

I have also found out that the current implementation of the Subscriptions\StorageManager is not quite optimal.
One example is Subscriptions\StorageManager::addSubscriberToTopic:

<?php
        $topic = $this->retrieveTopic($topicKey);
        $topic->push($subscriber->channel);
        $this->storeTopic($topicKey, $topic);

This has two downsides:

1. The cache gets overridden rather than pushed to, so a race condition could happen where two simultaneous writes could collide.
2. It reads all the data from cache for a push, which takes longer the more subscribers you have on a topic.

I have also written an optimised version for redis specifically. It uses sets for topics and pushes subscribers using the sadd command.

Would you want this to be integrated aswell and do you have a preferred way for that?
Do we have one RedisStorageManager and then as a fallback the general StorageManager?

[spawnia]

@stayallive has been doing great work on subscriptions lately and probably knows that part of Lighthouse better than i do. He also implemented a redis subscription storage: https://gist.github.com/stayallive/9b9fe8556faf9c8ba6f09ca54610e40f

The issues you point out seem quite severe, especially the racey adding of subscribers. I am not sure if we can do better with a generic cache implementation. Using redis seems like a great solution, it really has great primitives to handle concurrency.

I think the best path going forward is for you to create a PR so we can iterate on a better solution collaboratively.

[stayallive]

The issues you point out seem quite severe, especially the racey adding of subscribers. I am not sure if we can do better with a generic cache implementation. Using redis seems like a great solution, it really has great primitives to handle concurrency.

The only thing we could do is introduce locking so only 1 request can modify the subscribers: addSubscriberToTopic. However this is not supported on every cache driver (but many do since 7.x) so this is not a universal option either.

So there are a few things we can and possibly should do in the current cache driver:

• Detect lock support and use it if possible in the non-redis storage manager
• Add the Redis specific driver to prevent needing locks at all
• Document that the default storage manager only works in high concurrency when using a cache driver thats supports locking and the redis storage manager is advised

Your Redis driver looks fine but do take a look at mine because it supports the new expiration setting already so you might be able to take some ideas from there, also using the Laravel wrapper as much as possible so I don't need to serialize myself.

[thekonz]

I took inspiration from your driver and added the ttl aswell in the PR.
The only thing i am currently missing is the redis prefix, which I find weird for echo to say the least. All of a sudden, my channels were prefixed with laravel_database_, so I instead am telling people to remove the redis prefix.

Maybe I need to think about that a bit more and add the prefix back in.

[stayallive]

Yeah Redis is a prefixed driver. If you take a look at my implementation you’ll see me working with that so I would try and see of you can add that too since Redis prefixes are handy to have in case you don’t have access to multiple databases but need to run multiple apps.