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

Update to h11 0.8.1 #54

Closed
wants to merge 1 commit into from
Closed

Update to h11 0.8.1 #54

wants to merge 1 commit into from

Conversation

DaanDeMeyer
Copy link

Updates h11 to 0.8.0 and fixes a test regression. h11 0.8.0 does not
allow empty header values anymore so when the header value is empty
we now only add the header name without a ; and whitespace.

See #53

@DaanDeMeyer
Copy link
Author

All failed tests are due to a TypeError: unhashable type: 'bytearray' error at line 56 in connection.py. I have absolutely no knowledge about the internals of wsproto so any help on how to fix this would be appreciated

@DaanDeMeyer
Copy link
Author

When switching to h11 0.8.0 the _normed_header_dict function in wsproto suddenly receives an array of bytearrays instead of an array of strings. I can't find anything about this in the release notes of h11 0.8.0.

@DaanDeMeyer
Copy link
Author

I fixed the issue by converting the bytearrays in h11 0.8.0 to bytes (which does mean copying the bytearray).

Only a single test failed but I have no idea why (perhaps a flake?). @njsmith Can you have a look?

@njsmith
Copy link
Member

njsmith commented Mar 31, 2018

When switching to h11 0.8.0 the _normed_header_dict function in wsproto suddenly receives an array of bytearrays instead of an array of strings. I can't find anything about this in the release notes of h11 0.8.0.

Yeah, that sounds like h11 bug, and it's a plausible bug, because in 0.8.0 we removed some cast-to-bytes calls in the name of speed – maybe we need to add a few back. But, I can't seem to reproduce it locally! Can you give me more details about what version of Python you're using? It could be some change in the regexp module....

@njsmith
Copy link
Member

njsmith commented Mar 31, 2018

Also, I looked more closely at the test failure. It does indeed seem to be some unrelated flakiness. The failure is in autobahn server test 12.1.11, "Send 1000 compressed messages each of payload size 8192, auto-fragment to 256 octets. Use default permessage-deflate offer.", and the error is that wsproto is rejecting a message with the error: "encountered invalid UTF-8 while processing text message at payload octet index 8192"

So it sounds like this is a new variant of: #34 , crossbario/autobahn-testsuite#71

Previously this was only reported on later tests in the 12.* series, but the bug there is that the autobahn test is generating fixed-length random messages by making a long string of utf8 and truncating it, so sometimes the last byte ends in the middle of a utf8 character, which is exactly what wsproto is complaining about here.

@DaanDeMeyer
Copy link
Author

Should I make an issue in the h11 repository for the bytearray bug?

@njsmith
Copy link
Member

njsmith commented Mar 31, 2018

Sure

@njsmith
Copy link
Member

njsmith commented Apr 1, 2018

Note: the autobahn failure turns out to be weirder than I originally realized: crossbario/autobahn-testsuite#80

I guess we should set PYTHONHASHSEED=0 in the autobahn tests, as described here: https://tox.readthedocs.io/en/latest/example/basic.html#special-handling-of-pythonhashseed

@njsmith
Copy link
Member

njsmith commented Apr 15, 2018

I just released h11 v0.8.1 with the bytes fix in it, so it should be possible to fix this PR (or open a new one) to require v0.8.1 and drop the workarounds for python-hyper/h11#60

- h11 0.8.1 does not allow empty header values anymore so when the
header value is empty we only add the header name without a ; and
whitespace.
@DaanDeMeyer DaanDeMeyer changed the title Update to h11 0.8.0 Update to h11 0.8.1 Apr 23, 2018
@DaanDeMeyer
Copy link
Author

@njsmith Can you take a look at this? I think it's good for merging now.

@mgorny
Copy link

mgorny commented Jun 28, 2018

Ping. What's blocking this? We'd really like to avoid having to add 2-year old packages to Gentoo when a fix for current version of h11 is just around the corner.

@@ -47,7 +47,7 @@
'Programming Language :: Python :: Implementation :: PyPy',
],
install_requires=[
'h11 ~= 0.7.0', # means: 0.7.x where x >= 0
'h11 ~= 0.8.1', # means: 0.8.x where x >= 0

Choose a reason for hiding this comment

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

The comment here "where x >= 0" isn't strictly correct.
Should this just be a more relaxed ~= 0.8.0, or does it strictly need to be >= 0.8.1?

Copy link
Author

Choose a reason for hiding this comment

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

0.8.0 introduced errors which caused some wsproto tests to fail. 0.8.1 fixed the relevant bugs in h11 so it's required.

This was referenced Aug 14, 2018
@pgjones pgjones mentioned this pull request Sep 6, 2018
@Kriechi
Copy link
Member

Kriechi commented Sep 6, 2018

Thanks! 🎉 👍
Merged via 40d9586.

@Kriechi Kriechi closed this Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants