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.

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

Fix for 'Failed to parse headers' warning #1439

Merged
merged 9 commits into from Sep 19, 2018
4 changes: 2 additions & 2 deletions src/urllib3/exceptions.py
Expand Up @@ -236,8 +236,8 @@ def __init__(self, scheme):

class HeaderParsingError(HTTPError):
"Raised by assert_header_parsing, but we convert it to a log.warning statement."
def __init__(self, defects, unparsed_data):
message = '%s, unparsed data: %r' % (defects or 'Unknown', unparsed_data)
def __init__(self, defects):
Copy link
Member

Choose a reason for hiding this comment

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

I think it's important to maintain the unparsed_data attribute, it's useful to see where the parser finished parsing the raw bytes when there is a defect that stops HTTP parsing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed; reverted.

message = '%s' % (defects or 'Unknown')
super(HeaderParsingError, self).__init__(message)


Expand Down
10 changes: 2 additions & 8 deletions src/urllib3/util/response.py
Expand Up @@ -37,7 +37,6 @@ def is_fp_closed(obj):

def assert_header_parsing(headers):
"""
Asserts whether all headers have been successfully parsed.
Extracts encountered errors from the result of parsing headers.

Only works on Python 3.
Expand All @@ -56,14 +55,9 @@ def assert_header_parsing(headers):
type(headers)))

defects = getattr(headers, 'defects', None)
get_payload = getattr(headers, 'get_payload', None)

unparsed_data = None
if get_payload: # Platform-specific: Python 3.
unparsed_data = get_payload()

if defects or unparsed_data:
raise HeaderParsingError(defects=defects, unparsed_data=unparsed_data)
if defects:
Copy link
Member

Choose a reason for hiding this comment

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

Let's add back the unparsed_data getter. Let's continue checking if unparsed_data isn't a list and has len(unparsed_data) > 0 to preserve the same code path that was originally intended by the author.

Also add a comment about why we're ensuring that unparsed_data isn't a list, maybe reference EmailMessage.get_payload() documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed; I think the new changes address your points.

raise HeaderParsingError(defects=defects)


def is_response_to_head(response):
Expand Down
3 changes: 1 addition & 2 deletions test/test_exceptions.py
Expand Up @@ -32,7 +32,6 @@ def test_exceptions(self, exception):

class TestFormat(object):
def test_header_parsing_errors(self):
hpe = HeaderParsingError('defects', 'unparsed_data')
hpe = HeaderParsingError('defects')
Copy link
Member

Choose a reason for hiding this comment

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

Revert this change after implementing above changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


assert 'defects' in str(hpe)
assert 'unparsed_data' in str(hpe)
29 changes: 29 additions & 0 deletions test/with_dummyserver/test_socketlevel.py
Expand Up @@ -1328,6 +1328,35 @@ def test_header_without_colon_or_value(self):
])


@pytest.mark.skipif(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to skip here, we should be able to run these test cases on all Python versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll take your word on this. :)

issubclass(httplib.HTTPMessage, MimeToolMessage),
reason='Header parsing errors not available'
)
class TestOkayHeaders(SocketDummyServerTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Let's change the name of this test case to something like: TestHeaderParsingContentType? Same for the helper function, phrases like "okay" don't pinpoint the functionality being tested. It'd also be nice to have a comment within the unittest about why this is being tested separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.


def _test_okay_header_parsing(self, header):
self.start_response_handler((
b'HTTP/1.1 200 OK\r\n'
b'Content-Length: 0\r\n'
) + header + b'\r\n\r\n'
)

pool = HTTPConnectionPool(self.host, self.port, retries=False)
self.addCleanup(pool.close)

with LogRecorder() as logs:
pool.request('GET', '/')

for record in logs:
assert 'Failed to parse headers' not in record.msg

def test_header_text_plain(self):
self._test_okay_header_parsing(b'Content-type: text/plain')

def test_header_message_rfc822(self):
self._test_okay_header_parsing(b'Content-type: message/rfc822')


class TestHEAD(SocketDummyServerTestCase):
def test_chunked_head_response_does_not_hang(self):
self.start_response_handler(
Expand Down