Comparison between bytes and string in defining a frozenset throws exception

[chaofanhan]

https://github.com/python-hyper/hyper-h2/blob/3b0b241d79f5a9ff9382bbc038f84862e0d80abf/src/h2/utilities.py#L20-L26.

Hi, when a python process runs with a flag -bb, the above part of code will throw exception and make h2 not work. May I ask why we define both bytes and string in the frozenset? Is it possible to use only bytes or string? Frozenset will compare keys for deduplication.

[stroeder]

I agree this should be fixed, especially this module is used in many other stacks. Sets with mixed string types seem totally broken to me.

Maybe a custom set-class instead of frozenset?

[stroeder]

I tried to fix this but I give up for now due to lack of time. This seems really seriously broken! This whole module package serves as a good example why you should have typing.

IMO the devs have to decide where in the call-stack to decode the lower protocol data and refactor everything else above that. Especially remove the really strange kludges like h2.utility._custom_startswith().

[straz]

Even without -bb, this throws noisy warnings:

[BytesWarning] Comparison between bytes and string (.../h2/utilities.py:20)
[BytesWarning] Comparison between bytes and string (.../h2/utilities.py:29)
[BytesWarning] Comparison between bytes and string (.../h2/utilities.py:39)
[BytesWarning] Comparison between bytes and string (.../h2/utilities.py:46)
[BytesWarning] Comparison between bytes and string (.../h2/utilities.py:55)
[BytesWarning] Comparison between bytes and string (.../h2/utilities.py:60)

These warnings should at least be less noisy. Perhaps using warnings.simplefilter? reference

[pquentin]

This also affects urllib3 who runs tests with -bb to catch issues, but will have to stop to adopt h2.

[BYK]

So I want to fix this with a PR. It seems like this is done for efficiency reasons that said I also looked at hyper/hpack and it looks like we already need to convert everything to bytes before encoding. That said the _to_bytes() helper there converts everything into a string first so, we always go through that conversion. That means there's no reason to keep headers in bytes in h2 land for efficiency so we can convert everything into strings as a first step and continue life with string comparisons and string-sets.

Does that sound plausible? If yes, PR incoming; if no, please help me understand why :)

[BYK]

Pinging @Kriechi to get attention as I think otherwise this will just be missed.

[Kriechi]

@BYK I'm not familiar enough with this part of the code base to recommend such a larger refactoring. I think there is an implicit API expectation to the users of the h2 library that they can pass in both bytes and str headers, so we need to retain that backward-compatibility, until we cut a major release (which I don't see happening in the near future).

To focus on the issue itself: would separating the bytes vs. str checks solve the exception throwing? First check which class the header key and value are, and then compare them against the frozenset for its correct type? I could also see a custom frozenset implementation or class that wraps this behaviour into something like a type-agnostic compare function that does not throw an exception like the current code does.