amberframework / amber

A Crystal web framework that makes building applications fast, simple, and enjoyable. Get started with quick prototyping, less bugs, and blazing fast performance.
https://amberframework.org
MIT License
2.57k stars 205 forks source link

Allow Amber to handle multiple channels in Amber Redis Websockets Adapter #1265

Closed mixflame closed 1 year ago

mixflame commented 3 years ago

Description of the Change

Alternate Designs

Tried a bunch of stuff which didn't work but helped me build this

Benefits

Amber Redis Adapter for Websockets will now be able to serve pub sub subscriptions on multiple channels

Also now it cannot be unsubscribed because it will resubscribe on message like it used to

Possible Drawbacks (How to Use)

None. The style can maybe be refactored. I used a constant. There are no drawbacks.

Amber::Server.pubsub_adapter = Amber::WebSockets::Adapters::RedisAdapter

Tests included.

mixflame commented 3 years ago

+1 thank you for this codes

any time! :)

mixflame commented 2 years ago

Thanks Elias.... I will have the cleanup ASAP

mixflame commented 2 years ago

Elias,

Thanks for your review.

I cleaned up the code.

We can test it for production usage.

Thanks, Jon

mixflame commented 2 years ago

This is blocked by a crystal-redis merge confirming a vulnerability (that is patched) to an exploit (RCE) that I don't have the code for. With my crystal-redis patch, this cannot be exploited. I'm suspecting incorrect type routing through crystal-redis casts via strange inputs. This can be merged and the custom code can be used, but I hope to resolve the problem with crystal-redis and get an official OK for the entire deployment.

mixflame commented 2 years ago

I have confirmation that this is solid code from a while of testing on production. The error in crystal-redis was an error in my own Amber project. This can be merged. It's also now back to using the default crystal-redis from @steffanwille.

robacarp commented 1 year ago

This is a really ancient PR. A lot has changed in the redis ecosystem since then, it might be worth considering switching to https://github.com/jgaskins/redis -- it has much more stable connection management, uses the stdlib pool implementation, and is still actively maintained to boot. There was a bunch of discussion on Mosquito about this, and @jgaskins provided a lot of detailed comparison points about the two libraries.

Unfortunately during the course of that discussion I could not find a way to support both libraries -- one of them is module Redis and the other is class Redis which means both cannot even be installed in the same project and there is no obvious way to stub it out generically that doesn't pile on a bunch of maintainer overhead.

drujensen commented 1 year ago

@robacarp Ok, I am going to close this out. Thanks for providing the discussion about Redis. That will have impact on Amber for sure. We should open an issue to address that separately.