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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

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

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

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 15, 2018
@jasnell jasnell 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
Member Author

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

@maclover7 maclover7 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
const hooks = initHooks();
hooks.enable();

// The hooks.enable() must come before require('interna/test/binding')
Copy link
Contributor

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

hooks.enable();

// The hooks.enable() must come before require('interna/test/binding')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@jasnell jasnell force-pushed the http-parser-internal-binding branch from ccb4aba to 381cb2a Compare August 15, 2018 02:29
@jasnell jasnell requested a review from a team August 15, 2018 22:29
@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

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

@jasnell jasnell 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
Member Author

jasnell commented Aug 17, 2018

ping @nodejs/tsc

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

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

@joyeecheung joyeecheung mentioned this pull request Aug 17, 2018
Closed
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

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

@jasnell jasnell force-pushed the http-parser-internal-binding branch from dd2785a to 2a25ea1 Compare August 18, 2018 13:37
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

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

@jasnell jasnell requested a review from a team August 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 <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

Landed in 1744205

@jasnell jasnell closed this Aug 18, 2018
@joyeecheung joyeecheung mentioned this pull request Aug 20, 2018
Closed
@richardlau richardlau 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@devsnek devsnek devsnek approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@maclover7 maclover7 maclover7 approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs 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