Improve handling of server disconnects #112
Conversation
httpcore/_backends/sync.py
Outdated
| @@ -54,12 +54,15 @@ def start_tls( | |||
|
|
|||
| 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} | |||
There was a problem hiding this comment.
socket.error is a synonym for OSError in Python 3.
httpcore/_backends/trio.py
Outdated
| raise OSError(message) from None | ||
|
|
||
| if data == b"": | ||
| raise OSError("Server disconnected while attempting read") |
There was a problem hiding this comment.
very minor: you could reuse message here.
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… |
|
Are you happy to merge this? The fix looks good, even without tests it's better than not being in. |
|
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. :-) |
There was a problem hiding this comment.
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.
|
@JayH5 There's So hmm, actually I think I could just raise an |
|
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. |
|
@nawarnoori I just need to make a new pass on this to raise a |
|
@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 |
For the record, to test this as part of the test suite we would need these:
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 Shall we |
?
|
Thanks all! 🎉 |
Fixes #110, cc @nawarnoori
Now, the test setup detailed in #110 (comment) results in the following error in all cases:
httpcore._exceptions.ReadError: Server disconnected while attempting read