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.

Sign up for GitHub

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

Jump to bottom

Make llhttp default HTTP parser #24730

Closed
indutny opened this issue Nov 29, 2018 · 5 comments
Closed

Make llhttp default HTTP parser #24730

indutny opened this issue Nov 29, 2018 · 5 comments
Assignees
@addaleax

Comments

@indutny
Copy link
Member

indutny commented Nov 29, 2018

Is your feature request related to a problem? Please describe.

Latest node.js can be compiled with --experimental-http-parser flag. Eventually, we'd want to swap things around and make it enable llhttp parser by default and hide http_parser under --legacy-http-parser flag.

Describe the solution you'd like

Hide http_parser under the flag when we're ready.

Describe alternatives you've considered

No alternatives.

Let's discuss the earliest version of Node that we're going to try this in. http_parser is a major component and it would take a lot of testing to make sure that llhttp is on the par with it quality-wise.

cc @nodejs/release @nodejs/lts
@indutny indutny mentioned this issue Nov 29, 2018
Closed
@indutny
Copy link
Member Author

indutny commented Nov 29, 2018

cc @nodejs/http

@joyeecheung
Copy link
Member

Is it possible to make it a runtime switch first? My impression is that it should be possible with some small C++ refactoring in the Parser class and make the decision when we get it from the binding based on the CLI option which should be available at that point

@rvagg
Copy link
Member

rvagg commented Nov 29, 2018

related: I'm working on a sharedlibs builder in CI that'll test llhttp builds on all 11+ CI runs, so we'll soon be essentially mandating stability and quality for llhttp in parallel. Only covers one platform of course.

@addaleax addaleax mentioned this issue Nov 30, 2018
Closed
4 tasks
@addaleax
Copy link
Member

@joyeecheung → #24739

@ofrobots
Copy link
Contributor

IMHO, we should make llhttp the default in Node 12. If we do not do so, we will have to maintain and fix security issues in both http_parser and llhttp until 12 goes EOL in April 2022 at the earliest.

We will have 6 months of ecosystem testing available to us to harden llhttp before 12 goes LTS.

addaleax added a commit to addaleax/node that referenced this issue Dec 4, 2018
@addaleax
http: make parser choice a runtime flag
3587d21 
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

Refs: nodejs#24730
@addaleax addaleax self-assigned this Dec 5, 2018
danbev pushed a commit that referenced this issue Dec 6, 2018
@addaleax @danbev
http: make parser choice a runtime flag
aa943d0 
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: #24739
Refs: #24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
addaleax added a commit to addaleax/node that referenced this issue Dec 6, 2018
@addaleax
http: switch default parser to llhttp
790300d 
Refs: nodejs#24739
Fixes: nodejs#24730
@addaleax addaleax mentioned this issue Dec 6, 2018
Closed
3 tasks
BridgeAR pushed a commit that referenced this issue Dec 6, 2018
@addaleax @BridgeAR
http: make parser choice a runtime flag
4296e73 
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: #24739
Refs: #24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
This was referenced Dec 7, 2018
Closed
Closed
@addaleax addaleax mentioned this issue Dec 7, 2018
Closed
2 tasks
BridgeAR pushed a commit that referenced this issue Dec 7, 2018
@addaleax @BridgeAR
http: make parser choice a runtime flag
c8d5e31 
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: #24739
Refs: #24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
BridgeAR pushed a commit that referenced this issue Dec 7, 2018
@addaleax @BridgeAR
http: make parser choice a runtime flag
5ab22c8 
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: #24739
Refs: #24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@selaux selaux mentioned this issue Dec 21, 2018
Merged
9 tasks
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
@refack
http: make parser choice a runtime flag
dd48879 
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: nodejs#24739
Refs: nodejs#24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
@refack
http: switch default parser to llhttp
a843199 
Refs: nodejs#24739
Fixes: nodejs#24730

PR-URL: nodejs#24870
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@askmike askmike mentioned this issue Sep 17, 2019
Closed
@flotwig flotwig mentioned this issue Jan 7, 2020
Closed
@szmarczak szmarczak mentioned this issue Jun 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@addaleax addaleax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http: switch default parser to llhttp addaleax/node
5 participants
@ofrobots @indutny @rvagg @addaleax @joyeecheung