-
Notifications
You must be signed in to change notification settings - Fork 322
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
Replace http-parser with llhttp #651
Conversation
@ixti or @tarcieri: how do you feel about dropping Ruby 2.4 support with this change? Ruby 2.4 is no longer maintained (2.5 will EOL soon as well). IMO this seems like a good opportunity to drop support for old rubies. Right now https://github.com/metabahn/llhttp only supports Ruby 2.5 and up (explaining the failing checks above). I don't think there's any reason it couldn't support Ruby 2.4, but I'd prefer to only support maintained rubies. Let me know your thoughts. |
I'm fine with bumping the minimum Ruby version to 2.5, or even 2.6 if 2.5 is about to be EOL. |
Yeah with 5.x release I was hoping to bump the min ruby version. |
I'm gonna merge #652 and then will rebase&merge this PR |
Need to support Ruby 2.5 due to jRuby :(( |
Speaking of jRuby, I'm trying to debug the failure above but can't reproduce locally. Have you seen this before?
Not sure why it's trying to use |
It's not related to ffi-compiler. My branch with no other changes but ruby drop - fails on jruby as well o_O |
@tarcieri I propose to temporarily exclude jRuby from the CI and deal with it later |
@bryanp please rebase on master and I'll merge you PR. Will deal with jRuby later |
@ixti it might be time to consider migrating to using the Ruby 1.9 syntax |
@ixti done |
That is a new one to me... we have not shipped a "jrake" in years and years. There used to be a push (by others, not by us) to prefix every command installed in JRuby with a "j" like "jgem", "jrake", "jrails" and so on, so perhaps ffi-compiler got caught up in that? |
@headius the confusing thing is I don't see |
Thank you! |
@ixti Any idea when the next release might be pushed out? |
Builds on #639 to finish updating the response parser to use
llhttp
(using the ffi implementation for broader compatibility).I updated one spec as skipped as
llhttp
does not return an error likehttp-parser
does. Best I can tellllhttp
exhibits the correct behavior, which was also @midnight-wonderer's take here: #639 (comment). Let me know if you disagree and I'll start a discussion with the authors of thellhttp
c library.Resolves #604, #622.