fix: add TimeoutError handling in get_connection() by donbowman · Pull Request #1485 · redis/redis-py (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation16 Commits10 Checks37 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Pull Request check-list

Please make sure to review and check all of these items:

Does $ tox pass with this change (including linting)?
Does travis tests pass with this change (enable it first in your forked repo and wait for the travis build to finish)?
Is the new or changed code fully tested?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.

Description of change

In get_connection() we can implicitly call read on a connection. This can timeout of the underlying TCP session is gone. With this change we remove it from the connection pool and get a new connection.

Note: I purposely made this fix against the 3.5.3 tag since master is not quite ready to publish.

This pull request is marked stale. It will be closed in 30 days if it is not updated.

Hi @donbowman, will you have some time to complete the effort on this PR?

I see that the same pattern exists in two more places in the recent master branch -

In ConnectionPool class in the same file - the 'get_connection' method again
In async ConnectionPool - function 'ensure_connection'

Can you please add the TimeoutError there as well?

i guess, will it get looked at this time? i've rebased it several times in the last 4 years.

i guess, will it get looked at this time? i've rebased it several times in the last 4 years.

@donbowman Yes, I checked if we can merge it, and once we have the requested changes, it will be good to go :) Thank you for your time and patience :)

Is there any pointers on how to develop on this package?
It doesn't support poetry, i'm struggling to understand how to run its tests. I don't see any developer docs, maybe i'm not sure where to look?

It uses something called hatch? i did 'hatch shell' and then 'pip install -r dev_requirements', and 'hatch test', but it gives errors even on master., e.g. ModuleNotFoundError: No module named 'pytest_asyncio even though that package is installed in the venv.

Is there any pointers on how to develop on this package? It doesn't support poetry, i'm struggling to understand how to run its tests. I don't see any developer docs, maybe i'm not sure where to look?

It uses something called hatch? i did 'hatch shell' and then 'pip install -r dev_requirements', and 'hatch test', but it gives errors even on master., e.g. ModuleNotFoundError: No module named 'pytest_asyncio even though that package is installed in the venv.

The documentation on how to build the lib and run the tests is available in CONTRIBUTING.md

Unforunately those instructions do not work on clean master.

specifically, it has a conflict between redis-5.3.0b5 and redis-5.2.1

    python3 -m venv .venv
    . .venv/bin/activate
    pip install -r dev_requirements.txt
    pip install -e .[jwt]

fails:

Installing collected packages: redis
  Attempting uninstall: redis
    Found existing installation: redis 5.2.1
    Uninstalling redis-5.2.1:
      Successfully uninstalled redis-5.2.1
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
redis-entraid 0.4.0b2 requires redis~=5.3.0b3, but you have redis 5.2.1 which is incompatible.

so the dev_requirements redis-entraid is incompatible.

I worked around it by editing redis/init.py to say 5.3.0b3.

Is it expected that master will build/test? Just trying to baseline before I put work in.

ok requested changes made, tests pass locally. I was not sure what to do w/ the init.py, since the tests fail w/o bumping the version.

ok requested changes made, tests pass locally. I was not sure what to do w/ the init.py, since the tests fail w/o bumping the version.

In the pipeline, we uninstall the 5.03 dependency brought by entraid - quite uncomfortable for the contributors... I will add a separate PR to automate that and update the contribution guide because it will also be a problem for future relases. But for this change we should not yet increase the library version in init.py - we should keep the last stable version, 5.2.1.

ok have reverted the version change and manually uninstalled the entraid package.
please let me know if there is anything else.

ok have reverted the version change and manually uninstalled the entraid package. please let me know if there is anything else.

Nothing else is needed :) Thank you for your efforts!

petyaslavova pushed a commit to rohansingh/redis-py that referenced this pull request

Mar 25, 2025

fix: add TimeoutError handling in get_connection()

fix: add TimeoutError handling in get_connection() sync/async

fix: update version number to match test expectations
fix: revert version number to 5.2.1, manually uninstall entraid