Skip to content
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

IndexError on Connection Failure #2237

Closed
erewok opened this issue Jun 16, 2022 · 3 comments · Fixed by #2259
Closed

IndexError on Connection Failure #2237

erewok opened this issue Jun 16, 2022 · 3 comments · Fixed by #2259

Comments

@erewok
Copy link

erewok commented Jun 16, 2022

Version:
redis-py: 4.3.3
Redis Version: 6.0.14

Platform:

  • Kubernetes pod
  • Docker image
  • Python3.9

Description:

We are seeing IndexError when we reboot a redis server and our async Redis connection is closed and reconnect happens. The IndexError is located here in the codebase.

Traceback is like this:

  File "/usr/local/lib/python3.9/site-packages/sfdi_events_synd/clients/redis.py", line 81, in ping
    return await client.ping()
  File "/usr/local/lib/python3.9/site-packages/redis/asyncio/client.py", line 473, in execute_command
    conn = self.connection or await pool.get_connection(command_name, **options)
  File "/usr/local/lib/python3.9/site-packages/redis/asyncio/connection.py", line 1700, in get_connection
    await connection.connect()
  File "/usr/local/lib/python3.9/site-packages/redis/asyncio/connection.py", line 706, in connect
    raise ConnectionError(self._error_message(e))
  File "/usr/local/lib/python3.9/site-packages/redis/asyncio/connection.py", line 765, in _error_message
    f"Error {exception.args[0]} connecting to {self.host}:{self.port}. "
IndexError: tuple index out of range

It looks like our exception_args is empty?

        if len(exception.args) == 1:
            return f"Error connecting to {self.host}:{self.port}. {exception.args[0]}."
        else:
            return (
                f"Error {exception.args[0]} connecting to {self.host}:{self.port}. "
                f"{exception.args[0]}."
            )

We have been experimenting with ConnectionPooling and reconnect strategies, and this came up this morning on an experimental attempt to drop connections and reconnect in code that looks roughly like this:

@asynccontextmanager
async def redis_connection(pool):
    """
    Context manager for automatically closing connections on finish
    """
    connection = redis.Redis(
        connection_pool=pool, auto_close_connection_pool=False
    )
    yield connection
    await connection.close()


async def ping(pool):
    async with redis_connection(pool) as client:
        return await client.ping()

As noted above, we run this while rebooting our server to see if we can simulate failing connections and reconnect. We were expecting to see a ConnectionError instead of an IndexError.

@sileht
Copy link
Contributor

sileht commented Jun 21, 2022

In my case, the exception object was a https://docs.python.org/3/library/exceptions.html#ConnectionResetError

@sileht
Copy link
Contributor

sileht commented Jun 21, 2022

asyncio can raise the ConnectionResetError without creating an instance here:
https://github.com/python/cpython/blob/a31173c5ceb1708df687f942d714bdecae7cb759/Lib/asyncio/sslproto.py#L568

@erewok
Copy link
Author

erewok commented Jun 21, 2022

Wow. Nice find! That's super interesting.

sileht added a commit to sileht/redis-py that referenced this issue Jun 28, 2022
dvora-h pushed a commit to dvora-h/redis-py that referenced this issue Jul 27, 2022
dvora-h added a commit that referenced this issue Jul 28, 2022
* Add support for async graph

* linters

* fix docstring

* Use retry mechanism in async version of Connection objects (#2271)

* fix is_connected (#2278)

* fix: workaround asyncio bug on connection reset by peer (#2259)

Fixes #2237

* Fix crash: key expire while search (#2270)

* fix expire while search

* sleep

* docs: Fix a few typos (#2274)

* docs: Fix a few typos

There are small typos in:
- redis/cluster.py
- redis/commands/core.py
- redis/ocsp.py
- tests/test_cluster.py

Fixes:
- Should read `validity` rather than `valididy`.
- Should read `reinitialize` rather than `reinitilize`.
- Should read `farthest` rather than `farest`.
- Should read `commands` rather than `comamnds`.

* Update core.py

* async_cluster: fix concurrent pipeline (#2280)

- each pipeline should create separate stacks for each node

* Add support for TIMESERIES 1.8 (#2296)

* Add support for timeseries 1.8

* fix info

* linters

* linters

* fix info test

* type hints

* linters

* Remove verbose logging from `redis-py/redis/cluster.py` (#2238)

* removed the logging module and its corresponding methods

* updated CHANGES

* except block for RedisClusterException and BusyLoadingError removed

* removed unused import (redis.exceptions.BusyLoadingError)

* empty commit to re-trigger Actions workflow

* replaced BaseException with Exception

* empty commit to re-trigger Actions workflow

* empty commit to re-trigger Actions workflow

* redundant logic removed

* re-trigger pipeline

* reverted changes

* re-trigger pipeline

* except logic changed

* redis stream example (#2269)

* redis stream example

* redis stream example on docs/examples.rst

Co-authored-by: pedro.frazao <perl.pf@netcf.org>

* Fix: `start_id` type for `XAUTOCLAIM` (#2257)

* Changed start_id type for xautoclaim

* Added to changes

Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>

* Doc add timeseries example (#2267)

* DOC add timeseries example

* DOC add timeseries examples

* Apply suggestions

* Fix typo

Detention period => Retention period

Co-authored-by: Gauthier Imbert <gauthier@PC17>

* Fix warnings and resource usage problems in asyncio unittests (#2258)

* Use pytest-asyncio in auto mode
Remove overly genereric `pytestmark=pytest.mark.asyncio` causing lots of warning noise

* Use "Factories as Fixtures" test pattern for the `create_redis` fixture
this fixture is now async, avoiding teardown problems with missing event loops.

* Fix sporadic error on fast event loops, such as `--uvloop`

* Close connection, even if "username" was in kwargs
This fixes a resource usage warning in the async unittests.

* Do async cleanup of acl passwords via a fixture

* Remove unused import, fix whitespace

* Fix test with missing "await"

* Close pubsub objects after use in unittest
Use a simple fixture where possible, otherwise manually call pubsub.close()

* re-introduce `pytestmark=pytest.mark.asyncio` for python 3.6

* Use context manager to clean up connections in connection pool for unit tests

* Provide asynccontextmanager for python 3.6

* make `test_late_subscribe()` more robuste

* Catch a couple of additional leaked resources

* Graph - add counters for removed labels and properties (#2292)

* grpah - add counters for removed labels and properties

* added mock graph result set statistics

* docstrings for graph result set statistics

* format

* isort

* moved docstrings into functions

* cleaning up the readme and moving docs into readthedocs (#2291)

* cleaning up the readme and moving docs into readthedocs

* examples at the end as per pr comments

* async_cluster: fix max_connections/ssl & improve args (#2217)

* async_cluster: fix max_connections/ssl & improve args

- set proper connection_class if ssl = True
- pass max_connections/connection_class to ClusterNode
- recreate startup_nodes to properly initialize
- pass parser_class to Connection instead of changing it in on_connect
- only pass redis_connect_func if read_from_replicas = True
- add connection_error_retry_attempts parameter
- skip is_connected check in acquire_connection as it is already checked in send_packed_command

BREAKING:
- RedisCluster args except host & port are kw-only now
- RedisCluster will no longer accept unknown arguments
- RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url
- RedisCluster.require_full_coverage defaults to True
- ClusterNode args except host, port, & server_type are kw-only now

* async_cluster: remove kw-only requirement from client

Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>

* fix review comments

* fix

* fix review comments

* fix review comments

Co-authored-by: Chayim <chayim@users.noreply.github.com>
Co-authored-by: szumka <106675199+szumka@users.noreply.github.com>
Co-authored-by: Mehdi ABAAKOUK <sileht@sileht.net>
Co-authored-by: Tim Gates <tim.gates@iress.com>
Co-authored-by: Utkarsh Gupta <utkarshgupta137@gmail.com>
Co-authored-by: Nial Daly <34862917+nialdaly@users.noreply.github.com>
Co-authored-by: pedrofrazao <603718+pedrofrazao@users.noreply.github.com>
Co-authored-by: pedro.frazao <perl.pf@netcf.org>
Co-authored-by: Антон Безденежных <gamer392@yandex.ru>
Co-authored-by: Iglesys <g.imbert34@gmail.com>
Co-authored-by: Gauthier Imbert <gauthier@PC17>
Co-authored-by: Kristján Valur Jónsson <sweskman@gmail.com>
Co-authored-by: DvirDukhan <dvir@redis.com>
dvora-h added a commit that referenced this issue Nov 21, 2022
* fix is_connected (#2278)

* fix: workaround asyncio bug on connection reset by peer (#2259)

Fixes #2237

* Fix crash: key expire while search (#2270)

* fix expire while search

* sleep

* async_cluster: fix concurrent pipeline (#2280)

- each pipeline should create separate stacks for each node

* async_cluster: fix max_connections/ssl & improve args (#2217)

* async_cluster: fix max_connections/ssl & improve args

- set proper connection_class if ssl = True
- pass max_connections/connection_class to ClusterNode
- recreate startup_nodes to properly initialize
- pass parser_class to Connection instead of changing it in on_connect
- only pass redis_connect_func if read_from_replicas = True
- add connection_error_retry_attempts parameter
- skip is_connected check in acquire_connection as it is already checked in send_packed_command

BREAKING:
- RedisCluster args except host & port are kw-only now
- RedisCluster will no longer accept unknown arguments
- RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url
- RedisCluster.require_full_coverage defaults to True
- ClusterNode args except host, port, & server_type are kw-only now

* async_cluster: remove kw-only requirement from client

Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>

* More cherry picks to support latest module release

* Fix KeyError in async cluster - initialize before execute multi key commands (#2439)

* Fix KeyError in async cluster

* link to issue

* typo

* fixing lint

* fix json test

Co-authored-by: Mehdi ABAAKOUK <sileht@sileht.net>
Co-authored-by: Utkarsh Gupta <utkarshgupta137@gmail.com>
Co-authored-by: Chayim I. Kirshen <c@kirshen.com>
dvora-h added a commit that referenced this issue Nov 21, 2022
* Add support for async graph

* linters

* fix docstring

* Use retry mechanism in async version of Connection objects (#2271)

* fix is_connected (#2278)

* fix: workaround asyncio bug on connection reset by peer (#2259)

Fixes #2237

* Fix crash: key expire while search (#2270)

* fix expire while search

* sleep

* docs: Fix a few typos (#2274)

* docs: Fix a few typos

There are small typos in:
- redis/cluster.py
- redis/commands/core.py
- redis/ocsp.py
- tests/test_cluster.py

Fixes:
- Should read `validity` rather than `valididy`.
- Should read `reinitialize` rather than `reinitilize`.
- Should read `farthest` rather than `farest`.
- Should read `commands` rather than `comamnds`.

* Update core.py

* async_cluster: fix concurrent pipeline (#2280)

- each pipeline should create separate stacks for each node

* Add support for TIMESERIES 1.8 (#2296)

* Add support for timeseries 1.8

* fix info

* linters

* linters

* fix info test

* type hints

* linters

* Remove verbose logging from `redis-py/redis/cluster.py` (#2238)

* removed the logging module and its corresponding methods

* updated CHANGES

* except block for RedisClusterException and BusyLoadingError removed

* removed unused import (redis.exceptions.BusyLoadingError)

* empty commit to re-trigger Actions workflow

* replaced BaseException with Exception

* empty commit to re-trigger Actions workflow

* empty commit to re-trigger Actions workflow

* redundant logic removed

* re-trigger pipeline

* reverted changes

* re-trigger pipeline

* except logic changed

* redis stream example (#2269)

* redis stream example

* redis stream example on docs/examples.rst

Co-authored-by: pedro.frazao <perl.pf@netcf.org>

* Fix: `start_id` type for `XAUTOCLAIM` (#2257)

* Changed start_id type for xautoclaim

* Added to changes

Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>

* Doc add timeseries example (#2267)

* DOC add timeseries example

* DOC add timeseries examples

* Apply suggestions

* Fix typo

Detention period => Retention period

Co-authored-by: Gauthier Imbert <gauthier@PC17>

* Fix warnings and resource usage problems in asyncio unittests (#2258)

* Use pytest-asyncio in auto mode
Remove overly genereric `pytestmark=pytest.mark.asyncio` causing lots of warning noise

* Use "Factories as Fixtures" test pattern for the `create_redis` fixture
this fixture is now async, avoiding teardown problems with missing event loops.

* Fix sporadic error on fast event loops, such as `--uvloop`

* Close connection, even if "username" was in kwargs
This fixes a resource usage warning in the async unittests.

* Do async cleanup of acl passwords via a fixture

* Remove unused import, fix whitespace

* Fix test with missing "await"

* Close pubsub objects after use in unittest
Use a simple fixture where possible, otherwise manually call pubsub.close()

* re-introduce `pytestmark=pytest.mark.asyncio` for python 3.6

* Use context manager to clean up connections in connection pool for unit tests

* Provide asynccontextmanager for python 3.6

* make `test_late_subscribe()` more robuste

* Catch a couple of additional leaked resources

* Graph - add counters for removed labels and properties (#2292)

* grpah - add counters for removed labels and properties

* added mock graph result set statistics

* docstrings for graph result set statistics

* format

* isort

* moved docstrings into functions

* cleaning up the readme and moving docs into readthedocs (#2291)

* cleaning up the readme and moving docs into readthedocs

* examples at the end as per pr comments

* async_cluster: fix max_connections/ssl & improve args (#2217)

* async_cluster: fix max_connections/ssl & improve args

- set proper connection_class if ssl = True
- pass max_connections/connection_class to ClusterNode
- recreate startup_nodes to properly initialize
- pass parser_class to Connection instead of changing it in on_connect
- only pass redis_connect_func if read_from_replicas = True
- add connection_error_retry_attempts parameter
- skip is_connected check in acquire_connection as it is already checked in send_packed_command

BREAKING:
- RedisCluster args except host & port are kw-only now
- RedisCluster will no longer accept unknown arguments
- RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url
- RedisCluster.require_full_coverage defaults to True
- ClusterNode args except host, port, & server_type are kw-only now

* async_cluster: remove kw-only requirement from client

Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>

* fix review comments

* fix

* fix review comments

* fix review comments

Co-authored-by: Chayim <chayim@users.noreply.github.com>
Co-authored-by: szumka <106675199+szumka@users.noreply.github.com>
Co-authored-by: Mehdi ABAAKOUK <sileht@sileht.net>
Co-authored-by: Tim Gates <tim.gates@iress.com>
Co-authored-by: Utkarsh Gupta <utkarshgupta137@gmail.com>
Co-authored-by: Nial Daly <34862917+nialdaly@users.noreply.github.com>
Co-authored-by: pedrofrazao <603718+pedrofrazao@users.noreply.github.com>
Co-authored-by: pedro.frazao <perl.pf@netcf.org>
Co-authored-by: Антон Безденежных <gamer392@yandex.ru>
Co-authored-by: Iglesys <g.imbert34@gmail.com>
Co-authored-by: Gauthier Imbert <gauthier@PC17>
Co-authored-by: Kristján Valur Jónsson <sweskman@gmail.com>
Co-authored-by: DvirDukhan <dvir@redis.com>
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 a pull request may close this issue.

2 participants