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

Contribute to http2jp/hpack-test-case #71

Closed
dshafik opened this issue Oct 11, 2016 · 7 comments
Closed

Contribute to http2jp/hpack-test-case #71

dshafik opened this issue Oct 11, 2016 · 7 comments

Comments

@dshafik
Copy link
Contributor

dshafik commented Oct 11, 2016

Hi,

I think it would be great if this library could contribute results to the shared hpack test result repo: https://github.com/http2jp/hpack-test-case

This also provides a large set of example data to run tests against. I plan to contribute hyphper/h2-hpack results to it, and would love to compare.

@dshafik
Copy link
Contributor Author

dshafik commented Oct 11, 2016

Related to this, I see that, one of the test cases (#3 here) causes different output in python-hyper/hpack, although it can parse it back just fine:

hexlify(bytes(e.encode([(':method', 'GET'), (':scheme', 'http'), (':authority', 'k.yimg.jp'), (':path', '/images/top/sp2/cmn/logo-ns-130528.png')])))

Results in the following hex:

'82864187eabfa35332fd2b449b60d48e62a1849eb611589825353141e63ad52160b206c4f2f5d537'
-----------------------^

Notice the \x44 at the indicated location, in all other implementations, this is \x04. This implementation is able to decode both, with no difference in result. I believe this is the prefix, as created here.

I'm not sure why this is different, I attempted to look at some other hpack implementations but none of them are easy to read like this one. I have the same result in hyphper/h2-hpack not surprisingly, causing this, and I believe most of the other tests to fail.

@tatsuhiro-t
Copy link
Contributor

About 0x04 vs 0x44, it is "Literal Header Field without Indexing" vs "Literal Header Field with Incremental Indexing". At least for libnghttp2, it does not perform indexing for :path header field, this is because :path is usually not repeating much, and with our data, not indexing it gives us better compression ratio. You know, path could be very long these days.
It is completely fine to perform indexing of :path.

@dshafik
Copy link
Contributor Author

dshafik commented Oct 11, 2016

@tatsuhiro-t can you point me to the code for that behavior?

@Lukasa
Copy link
Member

Lukasa commented Oct 12, 2016

@dshafik Agreed, it'd be good to provide this data. I'm going to shove this on my to-do list (I'm only just back from holiday so I'm triaging everything that came up).

@Lukasa
Copy link
Member

Lukasa commented Oct 13, 2016

Alrighty, I've submitted my results upstream as http2jp/hpack-test-case#23.

@dshafik
Copy link
Contributor Author

dshafik commented Nov 7, 2016

These have been merged, so I'm closing out the bug. Thanks @Lukasa!

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

No branches or pull requests

3 participants