-
Notifications
You must be signed in to change notification settings - Fork 5k
fix: drain remaining bytes after [DONE] before closing response (#3440) #3520
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,6 +216,57 @@ def body() -> Iterator[bytes]: | |
| assert sse.json() == {"content": "известни"} | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize("sync", [True, False], ids=["sync", "async"]) | ||
| async def test_stream_drains_after_done( | ||
| sync: bool, | ||
| client: OpenAI, | ||
| async_client: AsyncOpenAI, | ||
| ) -> None: | ||
| """Regression test for #3440: after [DONE], remaining events should be drained. | ||
|
|
||
| The fix drains the existing iterator (not a new response.iter_bytes() call) | ||
| so that httpx doesn't raise StreamConsumed and the response reaches EOF | ||
| before close, preventing premature TCP FIN. | ||
|
|
||
| We track whether the body generator reached its final sentinel to verify | ||
| that the drain actually consumed the trailing event — not just that the | ||
| response was closed (which happens in the finally block regardless). | ||
| """ | ||
|
|
||
| consumed_after_done: list[bool] = [] | ||
|
|
||
| def body() -> Iterator[bytes]: | ||
| yield b'data: {"foo":true}\n' | ||
| yield b"\n" | ||
| yield b"data: [DONE]\n" | ||
| yield b"\n" | ||
| # Extra data after [DONE] that should be consumed by the drain | ||
| yield b'data: {"trailing":true}\n' | ||
| yield b"\n" | ||
| # Sentinel: only reached if the drain consumed all trailing events | ||
| consumed_after_done.append(True) | ||
|
|
||
| if sync: | ||
| response = httpx.Response(200, content=body()) | ||
| stream = Stream(cast_to=object, client=client, response=response) | ||
| # Consume the full stream — the drain should consume the trailing event | ||
| for _ in stream: | ||
| pass | ||
| assert response.is_closed | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This assertion is true even if the new drain loop is removed, because Useful? React with 👍 / 👎. |
||
| else: | ||
| response = httpx.Response(200, content=to_aiter(body())) | ||
| stream = AsyncStream(cast_to=object, client=async_client, response=response) | ||
| async for _ in stream: | ||
| pass | ||
| assert response.is_closed | ||
|
|
||
| # The sentinel is only appended if the body generator was fully consumed. | ||
| # Without the drain loop, the generator is garbage-collected when the | ||
| # response closes, so this assertion fails — proving the drain works. | ||
| assert consumed_after_done == [True] | ||
|
|
||
|
|
||
| async def to_aiter(iter: Iterator[bytes]) -> AsyncIterator[bytes]: | ||
| for chunk in iter: | ||
| yield chunk | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an upstream/proxy has already delivered the
[DONE]sentinel but then stalls or closes before EOF/chunk termination, this new drain keeps reading and anyReadTimeout/RemoteProtocolErrorfrom the best-effort cleanup now escapes after the stream is logically complete. Before this change the stream ended at[DONE]and thefinallyblock just closed the response, so users would not see a failure after receiving the complete stream; the async drain has the same issue. Consider making post-[DONE]draining best-effort so cleanup failures do not replace successful stream completion.Useful? React with 👍 / 👎.