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 hpack library #56

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Update hpack library #56

wants to merge 7 commits into from

Conversation

314eter
Copy link

@314eter 314eter commented Jul 1, 2019

This is a first attempt to replace the internal hpack library by ocaml-hpack.

Most of the changes are superficial, except for the header table size handling, which was not implemented before.

esy.json Outdated
@@ -32,7 +33,7 @@
},
"resolutions": {
"@opam/conf-libev": "esy-packages/libev:package.json#86d244e",
"@opam/conf-autoconf": "esy-packages/esy-autoconf:package.json#71a8836",
"@opam/conf-autoconf": "esy-packages/esy-autoconf:package.json#94f846f",
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for sending this along. I'd love to get this through CI to test what the impact of changes are, but it's choking on this autoconf change. Is there a reason why this particular resolution is included here?

Copy link
Author

Choose a reason for hiding this comment

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

Before the change, I got the following error:

echo "Updating man page config.guess.1"
Updating man page config.guess.1
PATH="../tests:../config:$PATH"; \
export PATH; \
/bin/sh /home/xxx/.esy/3_________________________________________________________________/b/opam__s__conf_autoconf-4e230a19/config/missing --run help2man \
    --include=./config.guess.x \
    --include=./common.x \
    --output=config.guess.1 config.guess
help2man: can't get `--help' info from config.guess
Try `--no-discard-stderr' if option outputs to stderr
make[1]: *** [Makefile:311: config.guess.1] Error 1
make[1]: Leaving directory '/home/xxx/.esy/3_________________________________________________________________/b/opam__s__conf_autoconf-4e230a19/man'
make: *** [Makefile:193: all-recursive] Error 1
error: command failed: 'make' (exited with 2)
esy-build-package: exiting with errors above...

Copy link
Author

Choose a reason for hiding this comment

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

I reverted the change, but it's still failing to build.

Copy link
Owner

Choose a reason for hiding this comment

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

that's annoying - it's flaky for some reason. I bet if I retry it it'll work...

Copy link
Owner

Choose a reason for hiding this comment

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

@314eter If you rebase on current master this issue should go away. Thanks for your patience.

@anmonteiro
Copy link
Owner

@314eter I was just waiting for the conformance tests to be run, and I was afraid this would happen, but there are some spec failures related to handling HPACK indexes and overflows.

Do you have a suggestion on how to fix these given how permissive your HPACK implementation is?

@314eter
Copy link
Author

314eter commented Jul 2, 2019

I already fixed them in 314eter/ocaml-hpack@8077a85, but the tests have to be run again.

@anmonteiro
Copy link
Owner

Oh cool, let me trigger a new CI run

@314eter
Copy link
Author

314eter commented Jul 3, 2019

I expected h2spec 4.3.1 to fail, but it didn't because of a bug that is fixed in 314eter/ocaml-hpack#10.

@anmonteiro
Copy link
Owner

@314eter I'm a little confused - so it will fail now if we update the dependency to include that new diff?

@314eter
Copy link
Author

314eter commented Jul 3, 2019

Yes, it will, because h2spec 4.3.1 interprets the rfc more strict than I do. It doesn't fail at the moment because it tests whether a table size update at the end of a header block is rejected. If it had tested with a table size update in the middle of a header block, it wouldn't have been rejected and the test would have failed.

I didn't merge the fix yet because another possible solution is to adapt to the h2spec interpretation, but that requires more work.

@hannesm
Copy link

hannesm commented Jul 6, 2023

I was curious about the state of this PR -- wouldn't it be nice to just use the existing hpack package and not vendor a copy inside of this repository? I fail to understand what is missing from this PR to land.

@anmonteiro
Copy link
Owner

@hannesm did you run into any issues that this PR fixes?

@314eter
Copy link
Author

314eter commented Jul 7, 2023

I remember I made some improvements, added fuzzing, and fixed a few bugs in ocaml-hpack after it was forked and vendored in ocaml-h2, and was still working on some other things. The bugs are probably still present in ocaml-h2. But I stopped working on hpack because everyone was using the h2 fork anyway.

If there is any interest, I can pick it up again.

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.

3 participants