Skip to content

http_parser: move process.binding('http_parser') to internalBinding#22329

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
Closed
jasnell wants to merge 1 commit into from
Closed

http_parser: move process.binding('http_parser') to internalBinding #22329

jasnell wants to merge 1 commit into from

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented Aug 15, 2018

Refs: #22160

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@jasnelljasnell added semver-major PRs that contain breaking changes and should be released in the next major version. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. labels Aug 15, 2018
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 15, 2018

@jasnell sadly an error occured when I tried to trigger a build :(

@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 15, 2018
@jasnelljasnell mentioned this pull request Aug 15, 2018
Closed
33 tasks
@devsnek
Copy link
Member

devsnek commented Aug 15, 2018•
edited
Loading

one interesting thing here is that people override process.binding('http_parser').HTTPParser for certain behaviours such as custom verbs or performance boosts for their workloads. i think we should look into exposing an api (maybe on http.request and http.createServer) to set a custom parser interface.

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 15, 2018

Yep, this one is definitely going to need the fallthrough support #22269 once I get that landed and we will need to be careful about fully deprecating this one.

maclover7
maclover7 reviewed Aug 15, 2018
View reviewed changes
Copy link
Contributor

@maclover7maclover7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small nits

test/async-hooks/test-httpparser.request.js Outdated
Copy link
Contributor

@maclover7maclover7Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: interna --> internal

test/async-hooks/test-httpparser.response.js Outdated
Copy link
Contributor

@maclover7maclover7Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@jasnelljasnellforce-pushed the http-parser-internal-binding branch from ccb4aba to 381cb2aCompareAugust 15, 2018 02:29
@jasnelljasnell requested a review from a teamAugust 15, 2018 22:29
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 15, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16473/

@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 15, 2018
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 17, 2018

ping @nodejs/tsc

BridgeAR
BridgeAR approved these changes Aug 17, 2018
View reviewed changes
Copy link
Member

@BridgeARBridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I prefer not to move the other functions inside of the main function. Instead, it should be possible to pass through the arguments and it's likely less churn that way.

@joyeecheungjoyeecheung mentioned this pull request Aug 17, 2018
Closed
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16533/

@jasnelljasnellforce-pushed the http-parser-internal-binding branch from dd2785a to 2a25ea1CompareAugust 18, 2018 13:37
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

Trying CI again: https://ci.nodejs.org/job/node-test-pull-request/16538/

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

Unrelated failure in CI...again... https://ci.nodejs.org/job/node-test-pull-request/16543/

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

Finally good CI. @nodejs/tsc ... PTAL

@jasnelljasnell requested a review from a teamAugust 18, 2018 19:43
jasnell added a commit that referenced this pull request Aug 18, 2018
@jasnell
http: move process.binding('http_parser') to internalBinding
1744205
Refs: #22160 PR-URL: #22329 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

Landed in 1744205

@jasnelljasnell closed this Aug 18, 2018
@joyeecheungjoyeecheung mentioned this pull request Aug 20, 2018
Closed
@richardlaurichardlau mentioned this pull request Oct 12, 2018
Closed
@gajus
Copy link

gajus commented Aug 8, 2019

one interesting thing here is that people override process.binding('http_parser').HTTPParser for certain behaviours such as custom verbs or performance boosts for their workloads. i think we should look into exposing an api (maybe on http.request and http.createServer) to set a custom parser interface.

Was this ever somehow addressed?

Currently facing issue in #22064 (comment)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

@joyeecheungjoyeecheungjoyeecheung approved these changes

@BridgeARBridgeARBridgeAR approved these changes

@trivikrtrivikrtrivikr approved these changes

+2 more reviewers

@maclover7maclover7maclover7 approved these changes

@devsnekdevsnekdevsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.http_parserIssues and PRs related to the HTTP Parser dependency or the http_parser binding.lib / srcIssues and PRs related to general changes in the lib or src directory.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jasnell@nodejs-github-bot@devsnek@gajus@addaleax@maclover7@joyeecheung@BridgeAR@trivikr