Question: Why creating a new StrictRedis object for each request?

[pior]

The connection pool is initialized at application configuration, that's the most important part for performance.

However, my understanding is that the StrictRedis class is thread-safe and could also be created once during application configuration and shared across requests.
Is there a reason to this?

[thruflo]

That may well be the case. I seem to remember the main client is thread safe whilst a pipeline instance isn't. Perhaps one way to configure with a single client would be to cache a configured client in the registry.

[wichert]

A pipeline or PubSub instance itself can indeed not be passed between threads, but that should not be a reason not to create multiple StrictRedis instances.

[thruflo]

Makes sense -- happy to take a PR!

[wichert]

The simplest implementation I can see is to just replace init.py with this:

import redis
from .config import DEFAULT_SETTINGS

def includeme(config):
    settings = DEFAULT_SETTINGS.copy()
    settings = config.get_settings()
    for key, value in DEFAULT_SETTINGS.items():
        settings.setdefault(key, value)
    settings['redis.max_connections'] = int(settings['redis.max_connections'])
    pool = redis.BlockingConnectionPool.from_url(settings['redis.max_connections'],
        db=settings['redis.max_connections'])
    r = redis.StrictRedis.from_url(connection_pool=pool)
    config.add_request_method(lambda request: r, 'redis', reify=True)

along with changing the default for redis.db to None. After this change you can drop hooks.py completely. Would a PR which essentially does that work for you?

[wichert]

Please note that this changes the API a little bit: instead of redus.unix_socket_path you will need to use redis.url = unix://path/to/socket, which is the standard way to handle UNIX paths.

[thruflo]

Re-reading the code in hooks.py I'm definitely feeling embarrassed by how overly complex / obfuscated it is. However, I'd be loathe to break anything that depends on the current implementation.

So... either perhaps a more conservative approach might be to just swap this line:

config.add_request_method(self.get_redis, 'redis', reify=True)

For:

r = self.get_redis(config)
get_redis = lambda request: r
config.add_request_method(get_redis, 'redis', reify=True)

That way the behaviour should stay exactly the same, aside from the overhead of instantiating every request?

[wichert]

I'ld like to use the from_url() method from the redis package instead of re-inventing that wheel locally, just to be sure that we are compatible with standard redis URLs. I can make that change in hooks.py though - I'll look at making a PR with that.