FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v8.x] cli: add --max-http-header-size flag by MylesBorins · Pull Request #25171 · nodejs/node · GitHub

/ node Public

[v8.x] cli: add --max-http-header-size flag - #25171

Closed
MylesBorins wants to merge 2 commits into
nodejs:v8.x-stagingfrom
MylesBorins:header-option-backport-8.x
ConversationCommits (2)ChecksFiles changed
Closed

[v8.x] cli: add --max-http-header-size flag #25171
MylesBorins wants to merge 2 commits into
nodejs:v8.x-stagingfrom
MylesBorins:header-option-backport-8.x

Conversation

MylesBorins commented Dec 21, 2018

Copy link
Copy Markdown
Contributor

backport of #24811

Did not backport 436f4de (not applicable to legacy option parser)
632f263 needed to be recreated from scratch PTAL... comments inline about major differences

deps: cherry-pick http_parser_set_max_header_size
74e376d 
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

PR-URL: nodejs#24811
Fixes: nodejs#24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

nodejs-github-bot commented Dec 21, 2018

Copy link
Copy Markdown
Collaborator

@MylesBorins build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2030/pipeline

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Dec 21, 2018
MylesBorins commented Dec 21, 2018
View reviewed changes
Comment thread doc/node.1 Outdated

MylesBorins Dec 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this needed to be rewritten to match style in 8.x PTAL and lmk if it looks right

Comment thread src/node.cc Outdated

MylesBorins Dec 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this is now exposed the way options were exposed in the past, PTAL

Comment thread src/node.h Outdated
Comment thread src/node_config.cc Outdated

MylesBorins Dec 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

as there is no longer getOptions I have appended the property of max_http_header_size to the process.bindings('config')... is this the best way to do it?

addaleax Dec 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If we need to expose this, then yes, this is probably the best way (do we need to, though?)

MylesBorins Dec 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

the option is exposed in 10.x and higher via getOptions... so was trying to keep the behavior equivalent.

Don't need to though, open to reverting this

Comment thread src/node_http_parser.cc Outdated

MylesBorins Dec 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this is the exact same as the 10.x PR aside from the options parsing

Comment thread test/sequential/test-http-max-http-headers.js Outdated

MylesBorins Dec 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

extra assert here to confirm that the value is on the process.binding('config')

MylesBorins force-pushed the header-option-backport-8.x branch from 174cf77 to 92ab0e0 Compare December 21, 2018 19:39
MylesBorins mentioned this pull request Dec 21, 2018
Merged
4 tasks

MylesBorins commented Dec 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This broke some stuff... digging in

edit: it was a missing comma 🤦🏽‍♀️

MylesBorins force-pushed the header-option-backport-8.x branch from 92ab0e0 to 73b53e1 Compare December 21, 2018 20:25

MylesBorins commented Dec 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

New CI after fix in bc2d3db

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

edit: arm failure rebuilding

https://ci.nodejs.org/job/node-test-commit-arm-fanned/5329/

cli: add --max-http-header-size flag
bc2d3db 
Allow the maximum size of HTTP headers to be overridden from
the command line.

co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: nodejs#24811
Fixes: nodejs#24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins force-pushed the header-option-backport-8.x branch from 73b53e1 to bc2d3db Compare December 21, 2018 20:39
MylesBorins mentioned this pull request Dec 21, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
deps: cherry-pick http_parser_set_max_header_size
d05760f 
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

Backport-PR-URL: #25171
PR-URL: #24811
Fixes: #24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
cli: add --max-http-header-size flag
d6c029d 
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: #25171
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins commented Dec 21, 2018

Copy link
Copy Markdown
Contributor Author

landed in 3812a99...d6c029d

mcollina reviewed Dec 21, 2018
View reviewed changes
Comment thread src/node.cc
} else if (strncmp(arg, "--v8-pool-size=", 15) == 0) {
v8_thread_pool_size = atoi(arg + 15);
} else if (strncmp(arg, "--max-http-header-size=", 23) == 0) {
max_http_header_size = atoi(arg + 23);

mcollina Dec 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this might be strtoul() instead?

MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
deps: cherry-pick http_parser_set_max_header_size
c713b4f 
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

Backport-PR-URL: #25171
PR-URL: #24811
Fixes: #24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
cli: add --max-http-header-size flag
665d415 
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: #25171
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 22, 2018
deps: cherry-pick http_parser_set_max_header_size
4fb5a1b 
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

Backport-PR-URL: #25171
PR-URL: #24811
Fixes: #24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 22, 2018
cli: add --max-http-header-size flag
693e362 
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: #25171
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina left review comments
addaleax addaleax approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL