Skip to content

Do not retry BaseExceptions that are not Exceptions - #698

Open
dchaudhari7177 wants to merge 1 commit into
jd:mainfrom
dchaudhari7177:fix/dont-retry-base-exceptions
Open

Do not retry BaseExceptions that are not Exceptions#698
dchaudhari7177 wants to merge 1 commit into
jd:mainfrom
dchaudhari7177:fix/dont-retry-base-exceptions

Conversation

@dchaudhari7177

Copy link
Copy Markdown

Closes #529.

Problem

retry_if_not_exception_type and retry_unless_exception_type both decide with not isinstance(e, self.exception_types), so anything outside the listed types is retried — including asyncio.CancelledError.

Measured on main, wrapping a 3-second coroutine in asyncio.wait_for(..., 0.3):

predicate attempts outcome
retry_if_not_exception_type(Boom) 2 wait_for returned normally
retry_unless_exception_type(Boom) 5 RetryError

Neither propagates the cancellation. The first is the worse of the two: wait_for does not even raise TimeoutError, so the caller cannot tell the timeout happened. KeyboardInterrupt, SystemExit and GeneratorExit are affected identically.

After:

predicate attempts outcome
retry_if_not_exception_type(Boom) 1 TimeoutError
retry_unless_exception_type(Boom) 1 TimeoutError

Fix

Both predicates now skip exceptions that do not derive from Exception. Those types derive from BaseException precisely so that blanket handlers leave them alone, and this is exactly the line retry_if_exception_type already draws through its Exception default — so the two families become consistent rather than one gaining a special case.

This follows @jackwildman's suggestion on the issue: reserve the control-flow types, and keep retry_if_exception_type(asyncio.CancelledError) as the way to retry one deliberately. That opt-in still works and has a test.

I fixed retry_unless_exception_type alongside the reported one because it is the same two-line predicate with the same failure mode; leaving it would make the behaviour depend on which of the two you picked. Happy to split it out if you would rather keep the PR to the reported predicate.

Behaviour change

retry_if_not_exception_type(X) / retry_unless_exception_type(X) no longer retry a BaseException outside X. Anyone depending on KeyboardInterrupt or a cancellation being retried would need retry_if_exception_type or a custom predicate — but that is the bug being fixed rather than a feature, and swallowing Ctrl-C is not something to preserve.

Tests

Five cases in TestRetryConditions:

  • both predicates return False for asyncio.CancelledError, KeyboardInterrupt, SystemExit and GeneratorExit
  • both still retry an ordinary Exception and still stop on their listed type
  • retry_if_exception_type(asyncio.CancelledError) still returns True, pinning the escape hatch

The two "skips base exceptions" tests fail on main — I checked by reverting only tenacity/retry.py.

Full suite: 171 passed, 12 subtests passed. ruff check and ruff format --check clean.

🤖 Generated with Claude Code

retry_if_not_exception_type and retry_unless_exception_type both decide
with `not isinstance(e, self.exception_types)`, so anything outside the
listed types is retried -- including asyncio.CancelledError. That
swallows a cancellation: under asyncio.wait_for the call is retried
instead of unwinding, so wait_for does not even raise TimeoutError.
KeyboardInterrupt, SystemExit and GeneratorExit are affected the same
way.

Skip exceptions that do not derive from Exception in both predicates.
Those types derive from BaseException precisely so blanket handlers
leave them alone, and retry_if_exception_type already skips them via its
Exception default -- which stays the explicit opt-in for retrying one on
purpose.

Closes jd#529

@jd jd left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This completely changes the contract of the API.

@dchaudhari7177

Copy link
Copy Markdown
Author

Fair — it does, and I should have proposed it rather than assumed it.

To be concrete about the alternative: the same fix can be opt-in and leave the current contract untouched, e.g.

retry_if_not_exception_type(Boom, retry_base_exceptions=False)

defaulting to True (today's behaviour), so nothing changes for existing users and anyone hitting the cancellation case has a supported way out. That's a smaller change than what's here and doesn't touch anyone's semantics.

The reason I'd still like something to land: as it stands retry_if_not_exception_type turns an asyncio.wait_for timeout into a normal return — the caller never sees TimeoutError and can't tell the timeout happened. That one seems worth addressing on its own, separately from the KeyboardInterrupt/SystemExit question, if the broader change is off the table.

Happy to go any of three ways — rework as the opt-in flag above, narrow it to just the wait_for/cancellation case, or close this if you'd rather #529 be solved differently. Your call; I won't push further either way.

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.

retry_if_not_exception_type swallows asyncio.CancelledError

2 participants