Repository navigation
fix(task): serialize sends with a lock so send() cannot hang - #190
Merged
Merged
Conversation
MessageSender used a queue drained by a background task. Once that loop stopped (write error, cancellation, or a send after close), queued and later sends awaited futures that were never settled. Replace the queue and loop with an asyncio.Lock: writes run in the caller's task, so errors and cancellation surface directly. After a failed write or close(), further sends raise ConnectionError. Also: - Connection._run_request drops a response that can't be sent because the connection already closed, instead of failing the task. - TaskSupervisor.shutdown() no longer re-raises errors from tasks that already failed and were reported by the error handlers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
MessageSenderpushed each frame onto a queue drained by a background loop task, andsend()awaited a future the loop settled. Once the loop stopped, nothing settled those futures, sosend()could wait forever:send()afterclose()enqueued behind the stop sentinel;TaskSupervisor.shutdown()) abandoned in-flight and queued sends.Fix
The sender only needs to serialize writes, so replace the queue + loop with an
asyncio.Lock.StreamWriter.write()buffers a whole frame synchronously, so frames can't interleave; the lock serializesdrain()in FIFO order. Writes now run in the caller's task, so errors and cancellation surface directly insend(). After a failed write orclose(), further sends raiseConnectionError(chained to the original write error).MessageSender(writer, supervisor)becomesMessageSender(writer); the only caller isConnection.Follow-ups exposed by sends no longer hanging:
Connection._run_request: a response that can't be sent because the connection already closed is dropped (debug log) instead of failing the task.TaskSupervisor.shutdown(): usesgather(..., return_exceptions=True), so it no longer re-raises errors from tasks that already failed and were reported by the error handlers (this madeConnection.close()raise intest_cancellation_cascades_between_sdk_peers).Behaviour change
close()no longer waits for queued frames to flush. Sends already inside the lock still complete; sends still waiting for the lock raiseConnectionError.Testing
tests/test_message_sender.py: write order, send after close, write failure fails waiting and later sends, cancelled send releases the lock.make check: all checks passed.uv run python -m pytest: 493 passed, 1 skipped (3 runs), excludingtests/http/test_websocket.pyandtests/http/test_ws_cookies.py. 5 tests in those two files fail in my local environment onmainas well, unrelated to this change.🤖 Generated with Claude Code