Normalize asyncio writer close races - #4217
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2132282bc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except (AttributeError, TypeError) as error: | ||
| raise ConnectionError("Connection closed while writing") from error |
There was a problem hiding this comment.
When a caller passes a malformed packed iterable, such as a custom generator or list containing non-bytes, StreamWriter.writelines() raises TypeError before any writer-close race is involved; this broad catch now turns that caller/data error into ConnectionError("Connection closed while writing"), disconnects, and can let retry logic re-run a non-transient bad command instead of surfacing the original error. Please only translate the specific closed-writer failure and let unrelated TypeErrors propagate as before.
AGENTS.md reference: AGENTS.md:L120-L123
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d7dd61c. The inner write wrapper now translates only AttributeError from a closed writer; caller/data TypeError values are allowed to propagate unchanged. Added a malformed packed-command regression test. The asyncio connection suite passes (67 passed, 1 skipped).
|
Please make sure resolve Codex comments first |
| try: | ||
| writer.writelines(command) | ||
| await writer.drain() | ||
| except (AttributeError, TypeError) as error: |
There was a problem hiding this comment.
the AttributeError/TypeError catch is broader than the None-writer case: pack_command aside, send_packed_command takes a caller-supplied iterable, so a bad element (str in the list) raises TypeError from writelines and now surfaces as ConnectionError, which retry.call_with_retry treats as retryable. that turns a programming error into a silent reconnect loop. since you already null-check writer above, is the except clause still buying anything beyond the writer-set-to-None-mid-await race?
There was a problem hiding this comment.
Addressed in d7dd61c. The broad TypeError catch was removed, so a caller-supplied invalid packed iterable is no longer reported as ConnectionError. The existing closed-writer coverage now models the AttributeError path explicitly.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit d7dd61c. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7dd61c5b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except AttributeError as error: | ||
| raise ConnectionError("Connection closed while writing") from error |
There was a problem hiding this comment.
For the reported Python 3.12 close race, the traceback in #3546 fails inside StreamWriter.writelines() as TypeError: 'NoneType' object is not callable, but this new handler only translates AttributeError; the actual failure still escapes as a raw TypeError and bypasses redis-py's ConnectionError retry path. Please classify that specific closed-transport TypeError without converting arbitrary caller/data TypeErrors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa2279f74a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4588fa1238
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c3a84066f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if str(error) != "'NoneType' object is not callable": | ||
| raise | ||
| raise ConnectionError("Connection closed while writing") from error |
There was a problem hiding this comment.
In the final code, this branch still identifies the closed-transport race solely by str(error). If a public send_packed_command() caller passes a malformed iterable that raises the same CPython text while writelines() consumes it (for example by calling a None callback), that caller/data bug is rewritten as ConnectionError, the connection is disconnected, and configured retry logic can replay a non-transient bad command instead of surfacing the original TypeError; please classify the transport race by its source/state rather than the exception message alone.
AGENTS.md reference: AGENTS.md:L120-L123
Useful? React with 👍 / 👎.
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for digging into this — the underlying problem is real. CPython clears
_SelectorSocketTransport._write_ready in _call_connection_lost on 3.12+, and writelines()
calls it without the _conn_lost guard that write() has, so a write on a peer-reset transport
raises a raw TypeError that our retry policy cannot classify. Mapping it to ConnectionError
in the async write path is the right layer.
Three things before we can merge it:
-
Classify by transport state rather than by exception message. str(error) comparisons
against CPython wording are not a stable contract, and they still rewrite a malformed
caller-supplied iterable into a retried ConnectionError (the open Codex comment).
Checkingwriter.transport is None or writer.transport.is_closing()before writelines()
is race-free — _force_close()/close() set _closing before scheduling _call_connection_lost,
and there is no suspension point between the check and the write. Re-checking is_closing()
inside a narrowexcept (AttributeError, TypeError)covers the drain() window. -
Please drop the _socket_is_empty()/process_invalidation_messages() change. Those methods
have no callers in the library or the test suite, and the added blanketexcept AttributeErrorconflicts with the deliberate "fail loudly if the private buffer API
changes" invariant in redis/_parsers/base.py. -
Add one regression test against real asyncio objects instead of mocks that supply the
CPython message text — e.g. connect to a local asyncio.start_server, call
conn._writer.transport.abort(),await asyncio.sleep(0), then send a command. The
current tests would keep passing even if the real path stopped being detected.
With those in place this should be ready for another review.
4 participants
Fixes #3546
Problem
When an asyncio transport is closed concurrently with a write, Python can raise a low-level
TypeErrorfromStreamWriter.writelines()after its internal_write_readycallback has been cleared. redis-py currently lets that rawTypeErrorescape, so the existing retry policy cannot recognize the broken connection as retryable.Solution
AttributeError/TypeErrorfailures into redis-py'sConnectionError.Tests
socket_timeout=Noneand finitesocket_timeoutpaths.invoke linters,compileall, andgit diff --checkpassed.The local integration skips are unchanged and are unrelated to this focused writer error path.
Note
Low Risk
Focused error normalization in asyncio connection I/O with targeted regression tests; behavior change is limited to exception type mapping on close races.
Overview
Fixes concurrent-close races where
StreamWriter.writelines()could raise rawAttributeError/TypeErrorinstead of a retryable connection failure.Write path: Adds
_send_packed_command()with a local writer reference, maps known close-race exceptions toConnectionError("Connection closed while writing"), and routes both timed and untimedsend_packed_command()through that helper.Read path:
_socket_is_empty()now guards a missing reader and maps buffer access failures toConnectionError("Connection closed while reading"), including during invalidation processing.Tests: New async tests cover closed-writer races (with/without
socket_timeout), closed reader during invalidation, and that unrelatedTypeError/AttributeErrorfrom bad commands still propagate.Reviewed by Cursor Bugbot for commit 7c3a840. Bugbot is set up for automated code reviews on this repo. Configure here.