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

Replace http-parser with llhttp #651

Merged
merged 4 commits into from Mar 4, 2021
Merged

Replace http-parser with llhttp #651

merged 4 commits into from Mar 4, 2021

Conversation

bryanp
Copy link
Contributor

@bryanp bryanp commented Mar 4, 2021

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 like http-parser does. Best I can tell llhttp 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 the llhttp c library.

Resolves #604, #622.

@bryanp
Copy link
Contributor Author

bryanp commented Mar 4, 2021

@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.

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2021

I'm fine with bumping the minimum Ruby version to 2.5, or even 2.6 if 2.5 is about to be EOL.

@ixti
Copy link
Member

ixti commented Mar 4, 2021

Yeah with 5.x release I was hoping to bump the min ruby version.

@ixti
Copy link
Member

ixti commented Mar 4, 2021

I'm gonna merge #652 and then will rebase&merge this PR

@ixti
Copy link
Member

ixti commented Mar 4, 2021

Need to support Ruby 2.5 due to jRuby :((

@bryanp
Copy link
Contributor Author

bryanp commented Mar 4, 2021

Speaking of jRuby, I'm trying to debug the failure above but can't reproduce locally. Have you seen this before?

exec: jrake: not found

Not sure why it's trying to use jrake—I don't see a reference to it in ffi-compiler.

@ixti
Copy link
Member

ixti commented Mar 4, 2021

It's not related to ffi-compiler. My branch with no other changes but ruby drop - fails on jruby as well o_O

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2021

@enebo @headius any idea what might be causing these jrake: not found errors which ostensibly appear to be coming out of ffi-compiler (even though that doesn't appear to directly reference jrake anywhere in its source code?)

@ixti
Copy link
Member

ixti commented Mar 4, 2021

@tarcieri I propose to temporarily exclude jRuby from the CI and deal with it later

@ixti
Copy link
Member

ixti commented Mar 4, 2021

@bryanp please rebase on master and I'll merge you PR. Will deal with jRuby later

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2021

lib/http/response/parser.rb:13:48: C: [Correctable] Style/HashSyntax: Use hash rockets syntax.
        @parser = LLHttp::Parser.new(@handler, type: :response)

@ixti it might be time to consider migrating to using the Ruby 1.9 syntax

@bryanp
Copy link
Contributor Author

bryanp commented Mar 4, 2021

@ixti done

@headius
Copy link

headius commented Mar 4, 2021

what might be causing these jrake: not found errors

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?

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2021

@headius the confusing thing is I don't see jrake referenced anywhere in the ffi-compiler source code, nor do any of its dependencies (of which it only has a couple) stick out at me in particular as potential culprits

@ixti ixti merged commit 8f8682c into httprb:master Mar 4, 2021
@ixti
Copy link
Member

ixti commented Mar 4, 2021

Thank you!

@bryanp
Copy link
Contributor Author

bryanp commented May 6, 2021

@ixti Any idea when the next release might be pushed out?

@tarcieri
Copy link
Member

See #659 and #660

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.

Time to drop http-parser?
5 participants