Skip to content

Conversation

@kirberich
Copy link
Contributor

Sorry again about my messing about with unrelated commits, this is a copy of the previous PR with the correct commits this time!

Hi!

As mentioned yesterday in #149, I found some problems with cachecontrol attempting to close the connection to redis when .close is called on the cache adapter, causing issues because the Redis object doesn't have a disconnect method.

I'm not an expert on redis, but as far as I can tell py-redis uses connection pooling by default, making it unnecessary to explicitly close connections (source: http://stackoverflow.com/questions/24875806/redis-in-python-how-do-you-close-the-connection)

I've coded up a quick fix that simply doesn't do anything when closing a redis cache, which I think is the correct behaviour. There might be cases where a user manages to instantiate redis without a connection pool, in which case closing the connection might be necessary, but I don't even know how to do that, so I'm hoping we can shelve that potential problem for now.

For reference, here's a quick example of code I've been using that caused problems for me with the original behaviour:

@contextmanager
def cache_session(session=None):
    """Context manager to easily apply caching to a session.

    An existing session can be passed in, otherwise
    a new Session is created.
    """

    session = session if session else Session()
    session.mount('http://', cache_adapter)
    session.mount('https://', cache_adapter)

    yield session

Robert Kirberich added 2 commits April 7, 2017 13:46
py-redis uses connection pooling, removing the need for handling connections explicitly.

There might still be special cases where closing a connection is needed, but the current behaviour breaks for standard use cases where the caching adapter gets closed (for example when using it in a context manager), so this seems like an easy improvement.
@ionrock ionrock merged commit c2ac282 into psf:master Aug 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants