Skip to content

asyncio: passing the sync redis.retry.Retry silently disables retries and failure callbacks #4262

Description

@Brumbelow

Description

Filed from the review of #4260 (see #4252): passing the sync redis.retry.Retry to any redis.asyncio client, pool, or connection is accepted silently and degrades every retry seam to a single attempt with no failure callback. It's an easy mistake — same class name, and the sync import is muscle memory — and the #4252 reproduction script did exactly this:

from redis.retry import Retry            # sync — wrong for asyncio, accepted silently
from redis.backoff import ExponentialBackoff
import redis.asyncio as aredis

pool = aredis.ConnectionPool.from_url(
    "redis://localhost:6379/0",
    retry=Retry(ExponentialBackoff(), 3),
    retry_on_error=[aredis.ConnectionError, aredis.TimeoutError],
)

Mechanism

Async call sites do await retry.call_with_retry(do, fail) where do is a coroutine function. The sync Retry.call_with_retry is a plain def: it calls do(), which for a coroutine function just creates the coroutine without raising, so the sync wrapper returns that coroutine immediately — its try/except around do() never observes the eventual exception. The caller then awaits the returned coroutine directly; when it fails:

  • the exception propagates on the first attempt (no retry loop, no backoff), and
  • the fail callback never runs — so e.g. Connection.check_health()'s _ping_failed (which disconnects the connection so the retried PING reconnects) and execute_command's disconnect-on-error handler are skipped.

conn.retry.get_retries() still reports the configured count and update_supported_errors() works, so the misconfiguration is invisible to the checks the #4252 reporter ran.

This affects every async retry seam: Connection.connect(), connect_check_health(), check_health(), Redis.execute_command(), and the cluster paths. Observed concretely while diagnosing #4252: the health-check PING failure on a server-closed connection surfaced unretried instead of disconnect-reconnect-retry.

The reverse direction (async redis.asyncio.retry.Retry passed to the sync client) fails loudly — commands return unawaited coroutine objects and Python emits "coroutine was never awaited" warnings — so it's the sync-into-async direction that needs a guard.

Suggested guard

Either, at the async boundary (redis.asyncio AbstractConnection.__init__ and the client/pool constructors that store a retry):

  1. Convert: if the object isn't the async Retry but is an AbstractRetry, rebuild it as redis.asyncio.retry.Retry(retry._backoff, retry._retries, retry._supported_errors) (optionally with a warning). Preserves the user's intent and silently repairs configurations that are broken today; or
  2. Reject: raise TypeError with a pointed message ("use redis.asyncio.retry.Retry with redis.asyncio clients").

Option 1 seems preferable since code affected today appears to work — its retries just never happen — and a hard error would turn silent misbehavior into a crash on upgrade.

Version

Present on master (3be919d3) and at least back through 8.0.x; not a regression, a long-standing silent-misconfiguration trap.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions