stefanwille / crystal-redis

Full featured Redis client for Crystal
MIT License
380 stars 61 forks source link

Add Redis::Pool class, with works with pool of connections, fixed #60 #62

Closed kostya closed 6 years ago

kostya commented 6 years ago

in spec "test multiconcurrent execution", try to replace Redis::Pool with Redis, you will see many concurrent errors.

kostya commented 6 years ago

i think it also should correct catch pool exceptions, and raise redis exceptions.

kostya commented 6 years ago

pool not means that it open all connections, but open new only when needed, and redis-server with timeout option, would just close not used for long.

stefanwille commented 6 years ago

Thanks again for this PR!

stefanwille commented 6 years ago

Regarding this comment:

in spec "test multiconcurrent execution", try to replace Redis::Pool with Redis, you will see many concurrent errors."

Yes, a Redis object is not meant to be used concurrently, and is expected to produce errors when done so. This is similar to other database connections - they are stateful, and therefore should not be used concurrently.

stefanwille commented 6 years ago

What you have done with transparently checking connections in and out for individual Redis commands is very cool, I have haven't seen this idea before anywhere.

stefanwille commented 6 years ago

What do you think?

kostya commented 6 years ago

What you have done with transparently checking connections in and out for individual Redis commands is very cool, I have haven't seen this idea before anywhere.

This is works in Celluloid (https://github.com/celluloid/celluloid/wiki/Pools), also transparent execute command on background pool, while for you it looks like you just call method

stefanwille commented 6 years ago

Great! I am pulling your PR now.