Fix HTTP parsing

[ixti]

☣️ DO NOT MERGE ☣️

This is a discussion starter for resolving #422

[ixti]

@httprb/core @janko-m @Bonias What do you think about us changing underlying http parser.

The problem with existing one is that it's locked to the old version of nodejs/http-parser. That can be fixed by updating underlying C version, but Java version of it is not maintained at all. So I think that the best option for us is to use FFI.

I'm not big fan of http-parser ffi wrapper but mostly due to it's API (it seems a bit ugly to me) and after all we end up by wrapper over wrapper. So probably if there are no strong objections against we can have our own http parser ffi version :D

[ixti]

Invited mastodon devs to participate in discussion as well.

//cc @Gargron

[Gargron]

cc @unarist @akihikodaki

[ixti]

So, the principal questions I propose to focus on are:

• if there are any objections against FFI
• if no objects, should we use existing gems for that or implement ourselves

[tarcieri]

I have no objections to FFI. It should provide a more consistent experience between MRI and JRuby. That said, I would recommend packaging the parser in the gem as opposed to requiring an external dependency. rbnacl-libsodium does this, and builds on both MRI and JRuby: https://github.com/cryptosphere/rbnacl-libsodium -- Tony Arcieri

[akihikodaki]

I have no objections to FFI.
I suggest to use the existing gem. Though its API may look odd, it is just conformant with the underlying the library and legitimate as a language binding.
I do not think wrapper over wrapper is really bad in this case. You just have different wrappers playing different roles: a wrapper providing APIs convenient for http.rb and a language binding.

[tarcieri]

Curious what @tmm1 thinks about converting the upstream http_parser.rb gem to use FFI and dropping the extant JRuby extension

[zanker]

It'll still work with JRuby via FFI though in that case, right?

…

On Sun, Oct 1, 2017 at 9:19 PM, Tony Arcieri ***@***.***> wrote: Curious what @tmm1 <https://github.com/tmm1> thinks about converting the upstream http_parser.rb gem to use FFI and dropping the extant JRuby extension — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#433 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATvvQ15_TtcrGUs77rhq1RRWUZfXBOKks5soGRQgaJpZM4Pp-TH> .

[tarcieri]

@zanker yes, although it will build the native code library at the time you install the gem

[zanker]

That sounds fine to me. Don’t think compiling is the end of the world unless we’re trying to target something that’s 100% java which seems unnecessary.

…

Sent from my iPhone

On Oct 1, 2017, at 22:26, Tony Arcieri ***@***.***> wrote: @zanker yes, although it will build the native code library at the time you install the gem — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[ixti]

@akihikodaki Well, I guess I have expressed myself a bit wrong. I'm totally fine with using existing gems even if it leads to wrapper over wrapper. I have only 2 problems with that particular gem:

• Naming. I find naming and some ruby API decisions pretty terrible. Not talking about things that are mapped to underlying library (Just general namespace and HttpParser::Parser.instance call makes me a bit sceptical)
• Consumption of body. I did not found streaming API there it seems like it fires body in one chunk after it was consumed (in case of non-chunked responses). (I was wrong - see next comment)

[ixti]

Okay. Excuse me. I was wrong. on_body actually fired everytime there's some data in body arrived.

[akihikodaki]

Oh, I see. HttpParser::Parser, HttpParser::Instance, HttpParser::Parser.new_instance… Indeed that sound bad.

[janko]

After finding out that http_parser.rb doesn't work on JRuby 9.2.0.0 anymore (#475), I realized why this PR is important 😃

It's great that there already exists the http-parser gem that builds upon Node.js' http-parser (just like the current http_parser.rb), but uses FFI instead of native extensions. I'm totally for using the http-parser gem, if any functionality is missing we can send PRs there.

Thanks @ixti for working on this!

[ixti]

Closed in favour of #489