-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
raise IncompleteReadError if only receive partial response #20888
Changes from 14 commits
9a5c337
bb40138
35549b6
729a873
8db8d69
36f3fd5
6237902
5db1dd7
89e8734
68259d2
92fd43b
8dfca0d
3da4453
aeab272
be24a3a
82335e5
3c3963a
8d17524
416253c
6e37282
5346036
bb4638d
59f0b34
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 |
---|---|---|
|
@@ -36,7 +36,8 @@ | |
from azure.core.configuration import ConnectionConfiguration | ||
from azure.core.exceptions import ( | ||
ServiceRequestError, | ||
ServiceResponseError | ||
ServiceResponseError, | ||
IncompleteReadError, | ||
) | ||
from . import HttpRequest # pylint: disable=unused-import | ||
|
||
|
@@ -79,6 +80,7 @@ def _read_raw_stream(response, chunk_size=1): | |
# https://github.com/psf/requests/blob/master/requests/models.py#L774 | ||
response._content_consumed = True # pylint: disable=protected-access | ||
|
||
|
||
class _RequestsTransportResponseBase(_HttpResponseBase): | ||
"""Base class for accessing response data. | ||
|
||
|
@@ -164,6 +166,10 @@ def __next__(self): | |
raise StopIteration() | ||
except requests.exceptions.StreamConsumedError: | ||
raise | ||
except requests.exceptions.ChunkedEncodingError as err: | ||
xiangyan99 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_LOGGER.warning("Incomplete download: %s", err) | ||
internal_response.close() | ||
raise IncompleteReadError(err, error=err) | ||
except Exception as err: | ||
_LOGGER.warning("Unable to stream download: %s", err) | ||
internal_response.close() | ||
|
@@ -289,7 +295,7 @@ def send(self, request, **kwargs): # type: ignore | |
""" | ||
self.open() | ||
response = None | ||
error = None # type: Optional[Union[ServiceRequestError, ServiceResponseError]] | ||
error = None # type: Optional[Union[ServiceRequestError, ServiceResponseError, IncompleteReadError]] | ||
|
||
try: | ||
connection_timeout = kwargs.pop('connection_timeout', self.connection_config.timeout) | ||
|
@@ -313,6 +319,7 @@ def send(self, request, **kwargs): # type: ignore | |
cert=kwargs.pop('connection_cert', self.connection_config.cert), | ||
allow_redirects=False, | ||
**kwargs) | ||
response.raw.enforce_content_length = True | ||
|
||
except (urllib3.exceptions.NewConnectionError, urllib3.exceptions.ConnectTimeoutError) as err: | ||
error = ServiceRequestError(err, error=err) | ||
|
@@ -323,6 +330,8 @@ def send(self, request, **kwargs): # type: ignore | |
error = ServiceResponseError(err, error=err) | ||
else: | ||
error = ServiceRequestError(err, error=err) | ||
except requests.exceptions.ChunkedEncodingError as err: | ||
error = IncompleteReadError(err, error=err) | ||
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. Is there a reason that this shouldn't just be a ServiceResponseError? 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. In our definition: """The request was sent, but the client failed to understand the response. My understanding is the server sends partial response should be a HttpResponseError? |
||
except requests.RequestException as err: | ||
error = ServiceRequestError(err, error=err) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
# coding: utf-8 | ||
# ------------------------------------------------------------------------- | ||
# Copyright (c) Microsoft Corporation. All rights reserved. | ||
# Licensed under the MIT License. See LICENSE.txt in the project root for | ||
# license information. | ||
# ------------------------------------------------------------------------- | ||
from azure.core.pipeline import AsyncPipeline | ||
from azure.core.pipeline.transport import ( | ||
HttpRequest, | ||
) | ||
from azure.core import AsyncPipelineClient | ||
from azure.core.exceptions import IncompleteReadError | ||
import pytest | ||
|
||
|
||
@pytest.mark.asyncio | ||
async def test_aio_transport_short_read_download_stream(port): | ||
url = "http://localhost:{}/errors/short-data".format(port) | ||
client = AsyncPipelineClient(url) | ||
with pytest.raises(IncompleteReadError): | ||
async with client: | ||
request = HttpRequest("GET", url) | ||
pipeline_response = await client._pipeline.run(request, stream=True) | ||
response = pipeline_response.http_response | ||
data = response.stream_download(client._pipeline) | ||
content = b"" | ||
async for d in data: | ||
content += d |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# coding: utf-8 | ||
# ------------------------------------------------------------------------- | ||
# Copyright (c) Microsoft Corporation. All rights reserved. | ||
# Licensed under the MIT License. See LICENSE.txt in the project root for | ||
# license information. | ||
# ------------------------------------------------------------------------- | ||
from azure.core import PipelineClient | ||
from azure.core.pipeline import Pipeline | ||
from azure.core.pipeline.transport import ( | ||
HttpRequest, | ||
RequestsTransport, | ||
) | ||
from azure.core.exceptions import IncompleteReadError | ||
import pytest | ||
|
||
|
||
def test_sync_transport_short_read_download_stream(port): | ||
url = "http://localhost:{}/errors/short-data".format(port) | ||
client = PipelineClient(url) | ||
request = HttpRequest("GET", url) | ||
with pytest.raises(IncompleteReadError): | ||
pipeline_response = client._pipeline.run(request, stream=True) | ||
response = pipeline_response.http_response | ||
data = response.stream_download(client._pipeline) | ||
content = b"" | ||
for d in data: | ||
content += d |
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.
Maybe we could rephrase as '...if peer closes the connection before we have received the complete message body.'
When I see 'sending', I think 'outgoing data'.