Skip to content

Redis backend connection fix #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 7, 2017

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