http-2
http-2 copied to clipboard
igrigorik
WIP: H2spec compliance fixes
Running H2spec and fixing the raised compliance issues.
It's a WIP, as I'm still going over them. I'll mark it as "done" once I fix them all, or fix a sufficient amount, or just get tired of doing it.
Coverage decreased (-0.7%) to 96.508% when pulling bfec1258fdcb91132518bffdb651be43f83e11d8 on HoneyryderChuck:h2spec-fixes into d52934f144db97fc7534e4c6025ed6ae86909b6a on igrigorik:master.
@igrigorik managed to make the local darwin h2spec run with no errors, and then I've realized it was version 1.5.0 . I've added h2spec's latest (2.2.0) to the CI build, which has ~100 more specs, and now there are 51 failing tests. Back to square one.
First off, thank you for working on this! 🙇Second, yikes.. 51 tests?
Stepping back, would it make sense to break this into multiple PR's? Worried that it might get unwieldy to review otherwise, and it would be nice to get the incremental wins without blocking on 100% success.
I guess 51 sounds rather bad, but in most cases just means that the lib doesn't handle certain edge cases, which make it a bit exploitable. But most ruby servers get proxied anyways, so I wouldn't sweat it much.
You could review it by commit, as I organized it (mostly) in a way that each commit handles a specific failure.
@igrigorik could you review this and when done, merge as is? I'm currently not finding the time to tackle the remaining 51 issues, and am also afraid that it will make this PR even more difficult to parse than it already is. However, I think that what was made already counts as significant improvement, as in:
- many spec compliance fixes have been taken care of;
- h2spec is now part of the CI (and doesn't break builds as is);
- h2spec is now easier to install locally;
I will come back to the remaining 51 tests after a while, and not only will I follow your suggestion of a PR per test, but this will also give an opportunity for others to contribute.
This is really great work. I didn't know h2spec was updated. I found it a bit buggy. I look forward to integrating some of these changes into my own upstream http/2 implementation.
@igrigorik left some more changes. If we can agree on the 2 remaining issues, then we could finally have this in master.
@ioquatix even after this is all done, we'll still be 51 specs behind. Hopefully we can count on your help to get that last mile :)
Looking at https://github.com/summerwind/h2spec/issues I'm not sure that h2spec is a good baseline. Basically, it's really great for evaluating the implementation, but as a target, it's got too many issues.
For example, the strict order it sometimes expect responses isn't actually required by the spec. But I don't believe this has ever been addressed: https://github.com/summerwind/h2spec/issues/93
I will have to try it out again with my latest implementation (https://github.com/socketry/http-protocol). I did have some automated testing with the latest h2spec, but even building it on Darwin was tricky if I recall correctly. How hard was it to build/use in travis?
I think it's a good-enough baseline, even with those pending issues. No tool is perfect :)
On the particular issue you mentioned, while I don't see exactly details about that particular interaction, it seems that the peer sends a HEADERS frame + DATA frame, which probably means that the request contains payload (content-length), and you shouldn't be sending responses before processing the data (unless Expect: 100-continue). I think that the spec covered well there.
About building in travis, I added a rake task to install it. It's not great, but it's self contained, and it works on my computer (mac) and in travis (linux), so I'd say it's good enough. Feel free to improve it though.
and you shouldn't be sending responses without processing the data (unless Expect: 100-continue). I think that the spec covered well there.
Can you point me at the relevant part in the RFC regarding this? I've often wondered if this behaviour is specified or not. Because it seems to me it should be fine to send a response without receiving the entire request body if you don't care about it.
I don't know if this enough for you, but https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html (4.4) states:
When a Content-Length is given in a message where a message-body is allowed, its field value MUST exactly match the number of OCTETs in the message-body. HTTP/1.1 user agents MUST notify the user when an invalid length is received and detected.
In order to detect the invalid length, you must receive it first.
This is a RFC for HTTP/1.1, but HTTP/2 states that it uses the same semantics, so I'd abide by this.
I can see the logic. Thanks. But it's a bit of a stretch. With HTTP/2 you can fail streams at any point... so reporting the error is not a matter of a 500 status code but perhaps a stream failure. I'd personally want to see this discussed explicitly, but I have scoured the RFCs and never seen anything which seems to explicitly deny sending a response before receiving the request body in it's entirety.
@HoneyryderChuck sorry about the delay, started reviewing this earlier but keep stumbling over scope and breadth of all the changes. Given some of the outstanding comments both from myself and others on the thread, I would recommend that we break apart this PR into smaller chunks.. Would you be willing to split this up, so we can tackle individual updates on case by case basis?
@igrigorik I get that this review requires more attention than the usual, but I don't think that breaking this down into smaller PRs would help that much. And frankly, my time has been very limited since the year started.
I don't know if you're reviewing all of the changes in bulk or if you're going commit-by-commit, but can I suggest the second approach if you're not doing it? I've just had another look at the list of commits, and the majority is relatively small and addresses a specific issue/scope. This way, you could simply cherry-pick them into master, and we could decrease the scope of this PR gradually. WDYT?
I get that I started with the wrong scope for this PR and now we're in this state, and your review time is pretty limited, but I still think that this adds more value than maintenance burden overall. I've just seen another compliance issue pop up which I think this PR already covers.
@tiago I can relate to your perspective, since my available time is also minimal at the moment.
@igrigorik I’m wondering if there is another collaborator that could jump in here and break this down into multiple PRs?
On Mar 3, 2019, at 05:38, Tiago [email protected] wrote:
@igrigorik I get that this review requires more attention than the usual, but I don't think that breaking this down into smaller PRs would help that much. And frankly, my time has been very limited since the year started.
I don't know if you're reviewing all of the changes in bulk or if you're going commit-by-commit, but can I suggest the second approach if you're not doing it? I've just had another look at the list of commits, and the majority is relatively small and addresses a specific issue/scope. This way, you could simply cherry-pick them into master, and we could decrease the scope of this PR gradually. WDYT?
I get that I started with the wrong scope for this PR and now we're in this state, and your review time is pretty limited, but I still think that this adds more value than maintenance burden overall. I've just seen another compliance issue pop up which I think this PR already covers.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.
Yeah, I was effectively thinking of cherry picking incremental bits.. If someone is willing to break this apart to make it easier to review and digest, that would be super helpful.
p.s. @HoneyryderChuck thanks for all the great work on this.
@igrigorik could you cherry-pick the following commits to master?
- https://github.com/igrigorik/http-2/pull/144/commits/ca444bca11739a7d0be1f72c9e89dd79fb238a27
- https://github.com/igrigorik/http-2/pull/144/commits/15d84417dba6aa3dd145f314bbbec78a96b28e6f
- https://github.com/igrigorik/http-2/pull/144/commits/79712f69d124c80e8e4d64abe78a2c5413e61110
- https://github.com/igrigorik/http-2/pull/144/commits/b171a6c6c9d33d967f802e040b47c83271b45aff
- https://github.com/igrigorik/http-2/pull/144/commits/49c754f250c7d6a9c28fe9773c5ef1b61895dbd9
- https://github.com/igrigorik/http-2/pull/144/commits/4b06d2812ddeba1c9d1189423b1445c1e078396a
- https://github.com/igrigorik/http-2/pull/144/commits/8852136158ab668a89bf76aa33595d6c0cec4baf
- https://github.com/igrigorik/http-2/pull/144/commits/ddf228343743920df09f54b0d2019b97a59310f9
- https://github.com/igrigorik/http-2/pull/144/commits/4db58c5c9a9c3473a4db2abf1a741ea66345ea7a
- https://github.com/igrigorik/http-2/pull/144/commits/8c50d263faf75ae6bfa5dcd2ab6d03360410ec7e
- https://github.com/igrigorik/http-2/pull/144/commits/5e043d0e3714880af85098a96309e69406b3de58
- https://github.com/igrigorik/http-2/pull/144/commits/aa9247736cd44ccf521be44c646790cf46cbb0e6
- https://github.com/igrigorik/http-2/pull/144/commits/56296b5b36256913b459606faebe85023ca29862
- https://github.com/igrigorik/http-2/pull/144/commits/b282ca9d5acdfa51d1bac631c5290d6b49d64201
- https://github.com/igrigorik/http-2/pull/144/commits/2a7a9b5fe2328d51bdde833536695c8a5eac02f6
- https://github.com/igrigorik/http-2/pull/144/commits/5d02c543312144012d93138730cc22d121d25a3b
- (this one has a
puts, consider removing it)
- (this one has a
- https://github.com/igrigorik/http-2/pull/144/commits/c2a50419f5943065a8a1887cc88625ee4d487cff
- https://github.com/igrigorik/http-2/pull/144/commits/d50cadfb014a0fad3e0f02fad62862da728973eb
- https://github.com/igrigorik/http-2/pull/144/commits/1d65a15766a5fee422df0299e09a8c70393777ba
- https://github.com/igrigorik/http-2/pull/144/commits/0c5d00ca85de8ff959fdb61cacfe8950bf769cbc
- https://github.com/igrigorik/http-2/pull/144/commits/215e78c7a22663f9cd71c45cbf4911b212dc3a10
- https://github.com/igrigorik/http-2/pull/144/commits/74555c89be2cbc9f3d42ebc4a9f9b8b4ca1c330e
All of the commits above are essentially bugfixes and a lot are one-liners. I left behind the ones involving ivars, as I assume those were the ones you're more concerned with.
- https://github.com/igrigorik/http-2/pull/144/commits/0997b6968552eabec899e5347b4ad5c076189c67
this one adds the rake task to install latest h2spec. It also adds the task to the CI build, so that we get some feedback.
I will have some time to work on this end of April, and if you could merge this low-hanging fruit, I could start working on breaking the remainder into separate PRs and see the remaining compliance issues.
@HoneyryderChuck Are you still interested in getting this merged? The author gave me permissions to the repo. We're looking to make improvements to this gem and release a new version.
@HoneyryderChuck I understand. http-2 is used with the AWS SDKs (current version and a planned version). Changing that away is difficult. I see you've been very active in improving it with your gem. I'd also like to propose a converge back. @igrigorik added me as a contributor here and I've been making a few fixes, maybe he would be open to adding you as a contributor as well as publish permissions? It seems like maybe a handoff is appropriate but I won't speak for him.
My proposal is this - which simplifies things I think - major version bump http-2 using http-2-next as the base.
Let me know what you think.
@mullermp thx for the reply!
current version and a planned version
Oh, that's interesting. Will the AWS API be allowing HTTP/2?
I'd also like to propose a converge back.
As mentioned in the other thread, I have no objections, just concerned about the "how", given the implied amount of work, as both repos diverged, in ways that mainline may not be comfortable accepting the changes from the fork. My (obviously biased) ideal approach would just be to hard-merge http-2-next into mainline, considering all spec compliance work done, performance improvements, RBS type defs, among other benefits. It's something I've never formally proposed for lack of activity in mainline and some fear of pushback. Not sure how you feel about it, but anything else would probably be a time sink I'd have very little time for. LMK what you think or were planning for, or whether you see some other approach / workaround.
maybe he would be open to adding you as a contributor as well as publish permissions?
Once both repos get merged somehow, I'd be open for it.
Oh, that's interesting. Will the AWS API be allowing HTTP/2?
We currently do this for streaming services such as Transcribe and Kinesis.
My (obviously biased) ideal approach would just be to hard-merge http-2-next into mainline, considering all spec compliance work done, performance improvements, RBS type defs, among other benefits.
Yep, I agree with this, as a major version bump (http-2 version 1.0 since latest gem is 0.11.0). Even better is if we can get this into stdlib ruby as net-http2 (though that gem is taken). Let me evaluate some options and talk to some folks.
Are you on the bundler slack? If you are, find me on there and let's chat on this.
Even better is if we can get this into stdlib ruby as net-http2 (though that gem is taken).
That's probably not desirable: stdlib is losing gems by the version, and for good reason (stdlib gems tend to silently die). net-http2 would probably be a weird name, considering this is a parser, and net-http is a full blown client. There's also (that I know of) no immediate benefit in having this a standard gem.
Are you on the bundler slack?
I'm not, is that by invite?
Try this invite (from google): https://join.slack.com/t/bundler/shared_invite/zt-1rrsuuv3m-OmXKWQf8K6iSla4~F1DBjQ
For anyone who comes across this, a new branch "main" was created that includes this work. https://github.com/igrigorik/http-2/pull/172