search

Krukov / cashews

Cache with async power
MIT License
367 stars 22 forks source link

Redis Client Side Connection Timeout #238

Open robert-schmidtke opened 1 month ago

robert-schmidtke commented 1 month ago

Hi @Krukov!

I'm using the Redis client-side caching feature, and every now and then obtaining the pubsub connection times out. So I read up in the code and noticed we could set socket_timeout, as this is used as the upper limit for the listen task to start. However, this socket_timeout is also used for all the regular cache reads, which I would rather not like to extend.

There's also a socket_connect_timeout, which is used by Redis client internally on obtaining a connection.

So would it make sense to have the listen task timeout be socket_connect_timeout instead? Or even socket_connect_timeout + 5 * socket_timeout, as there a ~5 execute_command operations used?

Alternatively, a dedicated option listen_timeout or pubsub_timeout (defaulting to socket_timeout or socket_connect_timeout) could be introduced?

https://github.com/Krukov/cashews/blob/7.1.0/cashews/backends/redis/client_side.py#L91 https://github.com/Krukov/cashews/blob/7.1.0/cashews/backends/redis/client_side.py#L122-L129

robert-schmidtke commented 1 month ago

I also found the timeout / wait_for_connection_timeout options in the super Redis backend, which may also be good candidates to use as timeouts for the listen task to start.

https://github.com/Krukov/cashews/blob/7.1.0/cashews/backends/redis/backend.py#L78

Krukov commented 4 weeks ago

Hi @robert-schmidtke

Yes, sure , I like this idea to have a dedicated option for the operations in the listen task and for waiting for the task started ( or use the timeout) . Would you like to make a PR or I can take it ?

Thanks

robert-schmidtke commented 4 weeks ago

Hey, thanks for offering to take it. I'm not sure I'll have the time soon, so please if you can, go ahead and add the feature to your preferences!