2.5.0 possible backward-incompatibility

[Kriechi]

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?

ref mitmproxy/mitmproxy#1671

[Lukasa]

Yeah, this was definitely a side-effect. Sorry. 😞 For what it's worth, the documentation does say that send_headers has a headers argument 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]

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]

Ok, sounds like the most sensible short-term fix. 😁

[Kriechi]

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]

😁 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]

@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]

@mhils I’m pretty sure hyper-h2 accepts a bytestring or a str, because the tests are pretty inconsistent about what they use.

[Lukasa]

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! 😎