Skip to content
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

Jump to bottom

Handle HTTP/1.1 half-closed connections gracefully. #641

Merged
merged 13 commits into from
Jul 7, 2023
Merged

Handle HTTP/1.1 half-closed connections gracefully. #641

merged 13 commits into from
Jul 7, 2023

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Dec 13, 2022 •
edited
Loading

Handle cases like HTTP 413, where the request write fails part way through sending, but a properly formed HTTP error response is then sent.

Closes #723

Originally prompted via encode/httpx#2503

@tomchristie tomchristie added the bug Something isn't working label Dec 13, 2022
@tomchristie
Copy link
Member Author

Test case failure is...

"ResourceWarning: Async generator 'httpcore._models.ByteStream.aiter' was garbage collected before it had been exhausted. Surround its use in 'async with aclosing(...):' to ensure that it gets cleaned up as soon as you're done using it."

Looks like this block...

httpcore/httpcore/_async/http11.py

Lines 133 to 135 in 6a97dad

async for chunk in request.stream:
event = h11.Data(data=chunk)
await self._send_event(event, timeout=timeout)

Needs an aclosing wrapper.

(No, I don't fully understand the generator clean-up stuff here.)

@zanieb
Copy link
Contributor

zanieb commented Dec 13, 2022

Very weird. I presume this is the WriteError interrupting iteration during _send_request_body?

https://docs.python.org/3/library/contextlib.html#contextlib.aclosing

Significantly, aclosing() supports deterministic cleanup of async generators when they happen to exit early by break or an exception

This pattern ensures that the generator’s async exit code is executed in the same context as its iterations (so that exceptions and context variables work as expected, and the exit code isn’t run after the lifetime of some task it depends on).

This is only available in Python 3.10+ though? You can try/finally/aclose I guess? I'm having a hard time creating a little reproduction.

We're not the only ones :) python/cpython#88684 (comment)

@mturoci mturoci mentioned this pull request Dec 14, 2022
Closed
@tomchristie
Copy link
Member Author

Related: #657 (?)

@cdeler
Copy link
Member

cdeler commented Jul 3, 2023

"ResourceWarning: Async generator 'httpcore._models.ByteStream.aiter' was garbage collected before it had been exhausted. Surround its use in 'async with aclosing(...):' to ensure that it gets cleaned up as soon as you're done using it."

I copied the test from this pr and see this error as well. Perhaps it's might be something else. Will try to dig into it

@tomchristie tomchristie changed the title If writing the complete request fails, then still attempt to read a response. Handle HTTP/1.1 half-closed connections gracefully. Jul 4, 2023
cdeler
cdeler reviewed Jul 5, 2023
View reviewed changes
httpcore/_sync/http11.py
with Trace("send_request_body", logger, request, kwargs) as trace:
self._send_request_body(**kwargs)
except WriteError:
# If we get a write error while we're writing the request,
Copy link
Member

@cdeler cdeler Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What happens if server just closes the request, but don't answer (I saw it in some proxies)? Read will fail, so we will have partially-closed connection?

Copy link
Member Author

Choose a reason for hiding this comment

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

The connection is closed and the client raises an exception...

httpcore.RemoteProtocolError("Server disconnected without sending a response.")

I've added a test case to demonstrate this.

cdeler
cdeler approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@cdeler cdeler left a comment

Choose a reason for hiding this comment

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

Looks good to me

@tomchristie tomchristie merged commit 04842b0 into master Jul 7, 2023
@tomchristie tomchristie deleted the handle-http-413-cases branch July 7, 2023 10:57
@tomchristie
Copy link
Member Author

Ah... Should have added to CHANGELOG.
Nevermind, reminder to self and all for future reviews.

@tomchristie tomchristie mentioned this pull request Jul 7, 2023
Merged
@karpetrosyan karpetrosyan mentioned this pull request Aug 1, 2023
Merged
1 task
@karpetrosyan karpetrosyan mentioned this pull request Aug 9, 2023
Open
magentalabs-serviceagent-1 pushed a commit to magenta-aps/ra-clients that referenced this pull request Nov 22, 2023
@carlbordum
fix: [#57426] timeout issues
94a83ac 
We're experiencing timeout errors in various integrations, but it does
not look like we ever send the request to OS2mo/Keycloak/etc. This MR
bumps httpx to bump httpcore to 0.18.0 to include this fix:
encode/httpcore#641.

If we're lucky, that's it. Otherwise, we are probably waiting for this
to be merged: encode/httpcore#802.
magentalabs-serviceagent-1 pushed a commit to magenta-aps/ra-clients that referenced this pull request Nov 22, 2023
@carlbordum
fix: [#57426] timeout issues
3a3dabc 
We're experiencing timeout errors in various integrations, but it does
not look like we ever send the request to OS2mo/Keycloak/etc. This MR
bumps httpx to a patched version which in turn depends on patched
version of httpcore which includes
encode/httpcore#802, which I suspect is our
issue.

Another related issue has also been fixed
(encode/httpcore#641), and is already included
in httpcore 0.18.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdeler cdeler cdeler approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle HTTP/1.1 half-closed connections gracefully.
3 participants
@tomchristie @zanieb @cdeler