Handle h11.Connection.next_event() RemoteProtocolException #524
Conversation
This file contains 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
There was a problem hiding this comment.
Thanks for this! Overall and after reading through the digging in #96, this looks sensible to me.
Some notes:
- I'm not able to reproduce the results I got in h11._util.RemoteProtocolError: can't handle event type ConnectionClosed when role=SERVER and state=SEND_RESPONSE #96 (comment). Could it be that somehow the issue is not surfacing on 3.8 / Uvicorn 0.10+?
- A
premature_close-type endpoint does yield the exact sameRemoteProtocolErrorthan the one listed in h11._util.RemoteProtocolError: can't handle event type ConnectionClosed when role=SERVER and state=SEND_RESPONSE #96:
>>> import httpx
>>>httpx.get("http://localhost:8000/premature_close")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/florimond/Developer/python-projects/httpx/httpx/api.py", line 125, in get
return request(
File "/Users/florimond/Developer/python-projects/httpx/httpx/api.py", line 84, in request
return client.request(
File "/Users/florimond/Developer/python-projects/httpx/httpx/client.py", line 845, in request
return self.send(
File "/Users/florimond/Developer/python-projects/httpx/httpx/client.py", line 908, in send
response.read()
File "/Users/florimond/Developer/python-projects/httpx/httpx/models.py", line 1097, in read
self._content = b"".join([part for part in self.stream()])
File "/Users/florimond/Developer/python-projects/httpx/httpx/models.py", line 1097, in <listcomp>
self._content = b"".join([part for part in self.stream()])
File "/Users/florimond/Developer/python-projects/httpx/httpx/models.py", line 1108, in stream
for chunk in self.raw():
File "/Users/florimond/Developer/python-projects/httpx/httpx/models.py", line 1136, in raw
for part in self._raw_stream:
File "/Users/florimond/Developer/python-projects/httpx/httpx/concurrency/base.py", line 159, in iterate
yield self.run(async_iterator.__anext__)
File "/Users/florimond/Developer/python-projects/httpx/httpx/concurrency/asyncio.py", line 260, in run
return self.loop.run_until_complete(coroutine(*args, **kwargs))
File "/Users/florimond/.pyenv/versions/3.8.0/lib/python3.8/asyncio/base_events.py", line 608, in run_until_complete
return future.result()
File "/Users/florimond/Developer/python-projects/httpx/httpx/dispatch/http11.py", line 152, in _receive_response_data
event = await self._receive_event(timeout)
File "/Users/florimond/Developer/python-projects/httpx/httpx/dispatch/http11.py", line 164, in _receive_event
event = self.h11_state.next_event()
File "/Users/florimond/Developer/python-projects/httpx/venv/lib/python3.8/site-packages/h11/_connection.py", line 420, in next_event
event = self._extract_next_receive_event()
File "/Users/florimond/Developer/python-projects/httpx/venv/lib/python3.8/site-packages/h11/_connection.py", line 369, in _extract_next_receive_event
event = self._reader.read_eof()
File "/Users/florimond/Developer/python-projects/httpx/venv/lib/python3.8/site-packages/h11/_readers.py", line 169, in read_eof
raise RemoteProtocolError(
h11._util.RemoteProtocolError: peer closed connection without sending complete message body (incomplete chunked read)This error would be caught and handled by this PR though (it raises a ConnectionClosed error), so I think we're on a good track anyway. I haven't followed the whole story though so I'll let others make a more informed call. :)
I saw your comment, and I didn't understand why this would cause an exception at all. I assumed you were doing something else, like killing the server at the right moment, as mentioned in earlier comments by others.
|
raise (new) ConnectionClosed exception if this occurs when socket is closed, otherwise reuse ProtocolError exception. Add test case for ConnectionClosed behavior. Resolves encode#96
|
address some issues with original pr.
Other then not really understanding how @florimondmanca test case ever triggered, I'm confident this PR addresses an uncaught exception issue. |
florimondmanca
approved these changes
Nov 19, 2019
|
pinging @tomchristie on this PR. it would be nice to get this trivial patch merged before it becomes stale. |
|
So, I just tried this out locally and it works fab. Also handles the case when a server is shutdown while we're receiving the response. So, merging! Thanks @toppk |
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.
here is a proposed fix. I wasn't too clear on the test suite (as usual :), but I was able to make a case that triggers the failure mode and test that the fix works as expected.
Addresses github issue #96
Fixes #96