Repository navigation
2.5.0 possible backward-incompatibility #367
Description
Hi,
we are getting a test failure in @mitmproxy with the latest h2 release (2.5.0):
Calling h2_conn.send_headers(event.stream_id, headers) with these headers
[(':status', '200'), ('priority_exclusive', True), ('priority_depends_on', 42424242), ('priority_weight', 42)]
results in the following traceback:
Traceback (most recent call last):
File ".../mitmproxy/test/mitmproxy/protocol/test_http2.py", line 77, in handle
if not self.server.handle_server_event(event, h2_conn, self.rfile, self.wfile):
File ".../mitmproxy/test/mitmproxy/protocol/test_http2.py", line 305, in handle_server_event
h2_conn.send_headers(event.stream_id, headers)
File ".../h2/connection.py", line 803, in send_headers
headers, self.encoder, end_stream
File ".../h2/stream.py", line 788, in send_headers
headers, encoder, hf, hdr_validation_flags
File ".../h2/stream.py", line 1119, in _build_headers_frames
encoded_headers = encoder.encode(headers)
File ".../hpack/hpack.py", line 229, in encode
for header in headers:
File ".../h2/utilities.py", line 302, in _reject_pseudo_header_fields
for header in headers:
File ".../h2/utilities.py", line 271, in _reject_connection_header
for header in headers:
File ".../h2/utilities.py", line 255, in _reject_te
for header in headers:
File ".../h2/utilities.py", line 69, in _secure_headers
for header in headers:
File ".../h2/utilities.py", line 437, in _strip_surrounding_whitespace
yield (header[0].strip(), header[1].strip())
AttributeError: 'bool' object has no attribute 'strip'
With earlier versions of h2 this code was working just fine.
I guess ('priority_exclusive', True) has to be a str now, and not bool?
I could not see any mentioning of that in the changlelog - if this was done by choice, or maybe it is a side effect?
Activity
Lukasa commentedon Oct 26, 2016
Yeah, this was definitely a side-effect. Sorry. 😞 For what it's worth, the documentation does say that
send_headershas aheadersargument that takes "An iterable of two tuples of bytestrings", so I think strictly speaking the interface never allowed what you were doing: it just so happened that by sheer good luck a cast to a bytestring happened somewhere and you got lucky.I suppose I'm willing to be convinced that we regressed an implied API here, but ultimately I'm not sure. What do you think?
mhils commentedon Oct 26, 2016
No problem, looks like we are at fault here. We shipped a patch release that constrains the h2 version, so things are fine on our end again. 😃
@Kriechi: Do you want to take over mitmproxy/mitmproxy#1671 and add the corresponding fixes? Guess that should be it and then can upgrade!
Lukasa commentedon Oct 26, 2016
Ok, sounds like the most sensible short-term fix. 😁
Kriechi commentedon Oct 27, 2016
Mea culpa - We will fix this on our side.
I guess the fine print in the h2 docs was just a bit too small for my eyes. 😛
Lukasa commentedon Oct 27, 2016
😁 No need for a mea culpa: this is the kind of bug that slips through in Python really easily. I think your suggestion in #368 is likely to be the best approach to dealing with this in the longer term.
mhils commentedon Oct 27, 2016
@Lukasa: Minor nitpick: The docs state that it should be a bytestring but also give
(':status', '1XX')as an example, which would be str. Shall hyper-h2 accept any of those, or shall we update the docstring?alexwlchan commentedon Oct 27, 2016
@mhils I’m pretty sure hyper-h2 accepts a bytestring or a str, because the tests are pretty inconsistent about what they use.
Lukasa commentedon Oct 28, 2016
In practice I think h2 will tolerate unicode strings: this is a historical remnant from the earlier versions of h2 which did headers as unicode everywhere. So in fact the docstring is out of date and should be updated. Patches welcome! 😎