etesync / server

The Etebase server (so you can run your own)
https://www.etesync.com
GNU Affero General Public License v3.0
1.56k stars 76 forks source link

Replace aioredis with redis-py #151

Closed Xiretza closed 2 years ago

Xiretza commented 2 years ago

aioredis has been merged into redis-py and will no longer be maintained as a separate project.

Fixes #150.

Xiretza commented 2 years ago

Unfortunately I don't have a redis setup to actually test this thoroughly, so it'd be great if someone else could give it a go.

tasn commented 2 years ago

Oh, good to know! Let me know once this has been properly tested. I also don't have time for at least a week. :(

jman-schief commented 2 years ago

Hi, I came here from https://github.com/etesync/server/issues/132, I had issues running etesync with a Redis endpoint configured. I have not yet the picture completely clear but I think @tasn is right that the Redis connection was not deeply tested. etesync crashes with a Redis endpoint configured.

@Xiretza I've tried your patch and dug a little bit further. I think we cannot yet use the old aioredis connection pool APIs for the time being (see redis-py async client documentation), the merge of aioredis into redis-py left some bits behind.

I've tried quickly hacking your patch as follows:

--- a/etebase_server/fastapi/redis.py
+++ b/etebase_server/fastapi/redis.py
@@ -12,12 +12,11 @@ class RedisWrapper:

     async def setup(self):
         if self.redis_uri is not None:
-            self.redis = await aioredis.create_redis_pool(self.redis_uri)
+            self.redis = await aioredis.from_url(self.redis_uri)

     async def close(self):
         if hasattr(self, "redis"):
-            self.redis.close()
-            await self.redis.wait_closed()
+            await self.redis.close()

and /at least/ I don't see crashes on startup/shutdown of etesync.

What do you think? makes sense?

That's all I can contribute for the time being, hope it helps! :)

tasn commented 2 years ago

It's not the end of the world not to use a pool, but it's much much better.

tasn commented 2 years ago

Actually, I take it back. I think with pub/sub, which we use, it's probably a big deal.

jman-schief commented 2 years ago

@tasn to further clarify my previous comment, I just meant that the old aioredis connection pool API is not available and one should probably switch to another one (as per the documentation link).

tasn commented 2 years ago

Yeah, I understood that @jman-schief, though thanks for the clarification. I just re-emphasized, that I think a pool is needed, or at least the code needs to be changed to reconnect every time (less than ideal).

tasn commented 2 years ago

OK, it looks like redis-py does indeed support pooling, we just need to slightly change how we create the object (as discussed above).

@Xiretza, got time to take a look, or should I see if I can adjust this PR?

tasn commented 2 years ago

I pushed the adjustments to Xiretza-redis-py on this repo. Can you pull the changes and add to this PR? I'll happily merge it after.

victor-rds commented 2 years ago

@tasn could you make a new release?

tasn commented 2 years ago

Done! Thanks @victor-rds for the reminder. In my head you're still tracking master. :)