Improve handling of server disconnects by florimondmanca · Pull Request #112 · encode/httpcore

florimondmanca · 2020-07-07T14:18:29Z

Now, the test setup detailed in #110 (comment) results in the following error in all cases:

httpcore._exceptions.ReadError: Server disconnected while attempting read

florimondmanca · 2020-07-07T14:21:26Z

 def read(self, n: int, timeout: TimeoutDict) -> bytes:
 read_timeout = timeout.get("read")
- exc_map = {socket.timeout: ReadTimeout, socket.error: ReadError}
+ exc_map = {socket.timeout: ReadTimeout, OSError: ReadError}


socket.error is a synonym for OSError in Python 3.

nawarnoori

Looks good, one minor comment, do you guys add tests? I suspect you'd have to spin up something to test properly against it.

nawarnoori · 2020-07-07T19:11:59Z

+ raise OSError(message) from None
+
+ if data == b"":
+ raise OSError("Server disconnected while attempting read")


very minor: you could reuse message here.

florimondmanca · 2020-07-08T12:32:01Z

do you guys add tests?

Yes, ideally we'd have a test for this. I did try something - spinning up a Uvicorn server and restart it while we're reading from the stream (which should trigger the error case). It feels pretty heavy just for this particular edge case. But I don't know how we'd go with mocking either, so…

nawarnoori · 2020-07-09T09:41:21Z

Are you happy to merge this? The fix looks good, even without tests it's better than not being in.

florimondmanca · 2020-07-09T09:42:59Z

I'm tempted to be okay with merging this in to issue a bug fix release too, but I'd like to hear thoughts from @encode/maintainers about this. :-)

JayH5

LGTM. My only question is why OSError was chosen? Seems unusual for that to be raised by something that's not stdlib, but I don't really know a better error type (EOFError? but that's also quite stdlib-specific).

[...] spinning up a Uvicorn server and restart it while we're reading from the stream (which should trigger the error case).

I think one other case for HTTP/1 where this may occur (if I'm understanding it right) is if the server disconnects due to a keepalive timeout.

florimondmanca · 2020-07-09T11:52:50Z

@JayH5 There's ConnectionAbortedError (a subclass of OSError) - I was hesitant to use it because we're mapping from OSError (so ReadError is raised in all cases) and it's not immediate knowledge that ConnectionAbortedError will be raised too.

So hmm, actually I think I could just raise an httpcore.ReadError directly. :-)

nawarnoori · 2020-07-10T15:26:12Z

Hi chaps, is there something I could do to help this along? I've patched the bug locally with a temporary hack but would really benefit from having this in the library.

florimondmanca · 2020-07-10T17:05:19Z

@nawarnoori I just need to make a new pass on this to raise a ReadError exceptions directly instead of OSError, and then this should be good for a final review… Unfortunately I don't think outside contributors can push to this branch, but if you'd like to speed things up feel free to bundle these ideas into a new PR.

nawarnoori · 2020-07-13T10:35:49Z

@florimondmanca I'm getting 403 on either pushing to this branch or pushing a new branch, but I've formatted the commit as a patch that I've attached here (had to add .txt to get github to upload it). It's very small, I've removed some exception mappings that I thought were unnecessary (since I couldn't see what would raise those exceptions) and just raise ReadError directly.

0001-Just-raise-ReadError-directly.patch.txt

JayH5

Would be nice to test but that seems difficult + the fix seems important.

florimondmanca · 2020-07-14T20:25:37Z

Would be nice to test

For the record, to test this as part of the test suite we would need these:

Spin up an HTTP/1.1 server locally (the bug was also reproducible on HTTP/2 but it's not an absolute requirement - it also occurred on HTTP/1.1).
Programmatically close the client connection on the server during the test (eg restart the server).
Synchronize all of this (eg server in one thread vs tests in the main thread).

We have / used to have something similar in HTTPX for connection pooling tests, but since at this point the test suite is very minimal (integration tests against a live website), we'd need some more work to be able to automatically test this properly.

Any case, I did some manual testing and my claim in the PR description holds: we get a proper ReadError in all cases instead of undefined behavior (eg loop forever on sock.recv()).

Shall we ?

lovelydinosaur

Yup, I'm good with this.

florimondmanca · 2020-07-15T09:44:03Z

Thanks all! 🎉

florimondmanca added 2 commits July 7, 2020 16:16

Handle client disconnects in the sync and asyncio cases
04d1ec4

Lint
171f390

florimondmanca commented Jul 7, 2020
View reviewed changes

Comment threadhttpcore/_backends/trio.py Outdated

florimondmanca commented Jul 7, 2020
View reviewed changes

nawarnoori approved these changes Jul 7, 2020
View reviewed changes

florimondmanca requested a review from a teamJuly 9, 2020 09:43

Merge branch 'master' into client-disconnects
3675cf1

JayH5 reviewed Jul 9, 2020
View reviewed changes

Comment threadhttpcore/_backends/trio.py Outdated

nawarnoori reviewed Jul 9, 2020
View reviewed changes

Comment threadhttpcore/_backends/sync.py Outdated

lovelydinosaur reviewed Jul 13, 2020
View reviewed changes

Comment threadhttpcore/_backends/asyncio.py Outdated

lovelydinosaur reviewed Jul 13, 2020
View reviewed changes

Comment threadhttpcore/_backends/sync.py Outdated

lovelydinosaur reviewed Jul 13, 2020
View reviewed changes

Comment threadhttpcore/_backends/trio.py Outdated

lovelydinosaur reviewed Jul 13, 2020
View reviewed changes

Comment threadhttpcore/_backends/trio.py Outdated

Update
26d7f21

JayH5 approved these changes Jul 14, 2020
View reviewed changes

lovelydinosaur approved these changes Jul 15, 2020
View reviewed changes

florimondmanca merged commit 44d4413 into masterJul 15, 2020

florimondmanca deleted the client-disconnects branch July 15, 2020 09:43

lovelydinosaur mentioned this pull request Aug 3, 2020
Version 0.10.0 #131
Merged

florimondmanca mentioned this pull request Aug 12, 2020
Improve handling of server disconnects on HTTP/2 #159
Closed

Kludex mentioned this pull request May 11, 2026
h11._util.RemoteProtocolError: can't handle event type ConnectionClosed when role=SERVER and state=SEND_RESPONSE pydantic/httpx2#29
Closed

Kludex mentioned this pull request May 11, 2026
0.14.x's sync client errors with ReadError w/ nginx 'chunked_transfer_encoding off' pydantic/httpx2#447
Closed
2 tasks

Uh oh!

Conversation

florimondmanca commented Jul 7, 2020

Uh oh!

Uh oh!

florimondmancaJul 7, 2020

Choose a reason for hiding this comment

Uh oh!

nawarnoori left a comment

Choose a reason for hiding this comment

Uh oh!

nawarnooriJul 7, 2020

Choose a reason for hiding this comment

Uh oh!

florimondmanca commented Jul 8, 2020

Uh oh!

nawarnoori commented Jul 9, 2020

Uh oh!

florimondmanca commented Jul 9, 2020

Uh oh!

JayH5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

florimondmanca commented Jul 9, 2020

Uh oh!

Uh oh!

nawarnoori commented Jul 10, 2020

Uh oh!

florimondmanca commented Jul 10, 2020

Uh oh!

nawarnoori commented Jul 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JayH5 left a comment

Choose a reason for hiding this comment

Uh oh!

florimondmanca commented Jul 14, 2020

Uh oh!

lovelydinosaur left a comment

Choose a reason for hiding this comment

Uh oh!

florimondmanca commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants