fix: add TimeoutError handling in get_connection() #1485

donbowman · 2021-05-20T22:06:42Z

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.

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.

codecov-commenter · 2021-05-20T22:13:00Z

Codecov Report

Merging #1485 (a7fb844) into master (9167a0e) will increase coverage by 12.64%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master    #1485       +/-   ##
===========================================
+ Coverage   79.35%   91.99%   +12.64%     
===========================================
  Files         108      108               
  Lines       27372    27373        +1     
===========================================
+ Hits        21720    25183     +3463     
+ Misses       5652     2190     -3462

Impacted Files	Coverage Δ
redis/connection.py	`86.36% <0.00%> (+0.47%)`	⬆️
tests/test_graph.py	`90.68% <0.00%> (+0.03%)`	⬆️
redis/commands/core.py	`82.99% <0.00%> (+0.16%)`	⬆️
redis/client.py	`88.76% <0.00%> (+0.76%)`	⬆️
redis/commands/parser.py	`67.77% <0.00%> (+1.11%)`	⬆️
redis/asyncio/connection.py	`83.85% <0.00%> (+1.68%)`	⬆️
tests/test_scripting.py	`93.67% <0.00%> (+3.16%)`	⬆️
tests/test_asyncio/conftest.py	`93.38% <0.00%> (+5.78%)`	⬆️
tests/conftest.py	`86.45% <0.00%> (+9.16%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9167a0e...a7fb844. Read the comment docs.

github-actions · 2022-05-21T00:18:05Z

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

donbowman · 2022-05-21T17:03:19Z

no comments?

github-actions · 2023-05-25T00:18:45Z

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

donbowman · 2023-05-25T03:16:39Z

no comments?

guess no

…to connection-pool-fix

donbowman · 2024-06-21T21:29:47Z

rebased again

petyaslavova · 2025-03-13T16:36:50Z

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?

donbowman · 2025-03-13T19:10:19Z

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

petyaslavova · 2025-03-14T07:24:45Z

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 :)

donbowman · 2025-03-16T22:10:12Z

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.

petyaslavova · 2025-03-17T07:40:53Z

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

donbowman · 2025-03-18T00:35:47Z

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.

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.

donbowman · 2025-03-18T01:38:58Z

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.

petyaslavova · 2025-03-18T08:09:38Z

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.

donbowman · 2025-03-18T14:46:33Z

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

petyaslavova · 2025-03-18T15:33:05Z

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!

* fix: add TimeoutError handling in get_connection() 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. * fix: add TimeoutError handling in get_connection() sync/async 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. * fix: update version number to match test expectations * fix: revert version number to 5.2.1, manually uninstall entraid

fix: add TimeoutError handling in get_connection()

59ab61f

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.

github-actions bot added the Stale label May 21, 2022

donbowman mentioned this pull request May 21, 2022

Retries do no work with asyncio #2179

Closed

github-actions bot removed the Stale label May 22, 2022

Merge remote-tracking branch 'upstream/master' into connection-pool-fix

a7fb844

github-actions bot added the Stale label May 25, 2023

github-actions bot removed the Stale label May 27, 2023

donbowman added 3 commits June 21, 2024 16:21

Merge branch 'master' into connection-pool-fix

08ad674

Merge branch 'connection-pool-fix' of github.com:Agilicus/redis-py in…

20f62da

…to connection-pool-fix

Merge branch 'master' into connection-pool-fix

50f2c98

Merge remote-tracking branch 'upstream/master' into connection-pool-fix

00751b0

donbowman and others added 3 commits March 17, 2025 21:19

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

05c12e0

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.

fix: update version number to match test expectations

302e89e

Merge branch 'master' into connection-pool-fix

b265df8

fix: revert version number to 5.2.1, manually uninstall entraid

42d0da9

petyaslavova approved these changes Mar 18, 2025

View reviewed changes

petyaslavova merged commit 6317ef5 into redis:master Mar 18, 2025
37 checks passed

petyaslavova added the maintenance Maintenance (CI, Releases, etc) label Apr 4, 2025

fix: add TimeoutError handling in get_connection() #1485

fix: add TimeoutError handling in get_connection() #1485

Uh oh!

Conversation

donbowman commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request check-list

Description of change

Uh oh!

codecov-commenter commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 21, 2022

Uh oh!

donbowman commented May 21, 2022

Uh oh!

github-actions bot commented May 25, 2023

Uh oh!

donbowman commented May 25, 2023

Uh oh!

donbowman commented Jun 21, 2024

Uh oh!

petyaslavova commented Mar 13, 2025

Uh oh!

donbowman commented Mar 13, 2025

Uh oh!

petyaslavova commented Mar 14, 2025

Uh oh!

donbowman commented Mar 16, 2025

Uh oh!

petyaslavova commented Mar 17, 2025

Uh oh!

donbowman commented Mar 18, 2025

Uh oh!

donbowman commented Mar 18, 2025

Uh oh!

petyaslavova commented Mar 18, 2025

Uh oh!

donbowman commented Mar 18, 2025

Uh oh!

petyaslavova commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

donbowman commented May 20, 2021 •

edited

Loading

codecov-commenter commented May 20, 2021 •

edited

Loading