Skip to content

Normalize asyncio writer close races #4217

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.

Sign up for GitHub

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

Open
lh0156 wants to merge 2 commits into
base: master
Choose a base branch
from
+42 −4
Open

Normalize asyncio writer close races #4217

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2132282
Normalize asyncio writer close races
lh0156 Jul 26, 2026
d7dd61c
Preserve packed command TypeErrors
lh0156 Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions redis/asyncio/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,8 +1143,14 @@ async def check_health(self):
)

async def _send_packed_command(self, command: Iterable[bytes]) -> None:
self._writer.writelines(command)
await self._writer.drain()
writer = self._writer
if writer is None:
raise ConnectionError("Connection closed while writing")
try:
writer.writelines(command)
await writer.drain()
except (AttributeError, TypeError) as error:

@Sanjays2402 Sanjays2402 Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@lh0156 lh0156 Jul 31, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

raise ConnectionError("Connection closed while writing") from error

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve non-I/O TypeErrors from writes

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 👍 / 👎.

@lh0156 lh0156 Jul 31, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@cursor cursor Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TypeError from transport close race not caught

High Severity

The PR's stated purpose is to convert writer-close TypeError failures into ConnectionError so the retry mechanism can handle them, but _send_packed_command only catches AttributeError, not TypeError. The transport-close-race TypeError (the core problem from issue #3546) passes through uncaught, hits the except BaseException handler in send_packed_command, and is re-raised as a raw TypeError. Since the retry mechanism's supported_errors only includes ConnectionError and TimeoutError, the TypeError is still not retryable — leaving the original bug unfixed.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d7dd61c. Configure here.

Comment on lines +1152 to +1153

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Handle the actual closed-transport TypeError

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 👍 / 👎.


async def send_packed_command(
self, command: Union[bytes, str, Iterable[bytes]], check_health: bool = True
Expand All @@ -1164,8 +1170,7 @@ async def send_packed_command(
self._send_packed_command(command), self.socket_timeout
)
else:
self._writer.writelines(command)
await self._writer.drain()
await self._send_packed_command(command)
except asyncio.TimeoutError:
await self.disconnect(nowait=True)
raise TimeoutError("Timeout writing to socket") from None
Expand Down
16 changes: 16 additions & 0 deletions tests/test_asyncio/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,22 @@ def test_connection_default_parser_matches_default_protocol():
assert conn.protocol == 3


@pytest.mark.parametrize("socket_timeout", [None, 1])
async def test_closed_writer_during_write_is_connection_error(socket_timeout):
conn = Connection(socket_timeout=socket_timeout)
conn._reader = mock.Mock()
conn._writer = mock.Mock()
conn._writer.writelines.side_effect = TypeError("'NoneType' object is not callable")
conn.disconnect = mock.AsyncMock()

with pytest.raises(ConnectionError, match="Connection closed while writing"):
await conn.send_packed_command([b"PING\r\n"], check_health=False)

conn.disconnect.assert_awaited_once_with(nowait=True)
conn._reader = None
conn._writer = None


@pytest.mark.parametrize(
("buffer", "eof", "expected"),
[
Expand Down