Skip to content

http2: add maxHeaderSize option to http2#33636

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
preyunk wants to merge 7 commits into from
Closed

http2: add maxHeaderSize option to http2 #33636

preyunk wants to merge 7 commits into from

Conversation

@preyunk
Copy link
Contributor

@preyunkpreyunk commented May 29, 2020

add maxHeaderSize to http2 as an alias for maxHeaderListSize.

Fixes: #33517

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label May 29, 2020
himself65
himself65 requested changes May 29, 2020
View reviewed changes
Copy link
Member

@himself65himself65 left a comment

Choose a reason for hiding this comment

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

tests needed

@preyunkpreyunk marked this pull request as draft May 29, 2020 11:45
rexagod
rexagod reviewed May 29, 2020
View reviewed changes
rexagod
rexagod suggested changes May 29, 2020
View reviewed changes
Copy link
Member

@rexagodrexagod left a comment

Choose a reason for hiding this comment

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

Please add tests for test-http2-session-settings.js, test-http2-client-settings-before-connect.js, and test-http2-too-large-headers.js as well.

@preyunk
Copy link
ContributorAuthor

preyunk commented May 29, 2020•
edited
Loading

Guess I forgot to run the linter before pushing, will push the changes after fixing the issues.
Also my origin HEAD was behind my local branch's HEAD so I followed the steps written in pull-requests.md and did a git push --force-with-lease origin my-branch

@preyunkpreyunkforce-pushed the http2-max-header branch 2 times, most recently from cc3ffed to 2f3782cCompareMay 30, 2020 14:58
@preyunkpreyunk requested a review from himself65May 30, 2020 19:12
rexagod
rexagod suggested changes May 30, 2020
View reviewed changes
himself65
himself65 reviewed May 31, 2020
View reviewed changes
himself65
himself65 reviewed May 31, 2020
View reviewed changes
Copy link
Member

@himself65himself65 left a comment

Choose a reason for hiding this comment

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

What's more, we need to add the doc for the new features

@preyunk
Copy link
ContributorAuthor

preyunk commented May 31, 2020•
edited
Loading

@himself65 any guidelines as what should be written for maxHeaderSize in docs? I am thinking like mentioning it as an alias for maxHeaderListSize.

@BridgeARBridgeARforce-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72CompareMay 31, 2020 12:18
@himself65
Copy link
Member

himself65 commented May 31, 2020

@himself65 any guidelines as what should be written for maxHeaderSize in docs? I am thinking like mentioning it as an alias for maxHeaderListSize.

https://github.com/nodejs/node/blob/master/doc/guides/doc-style-guide.md

@preyunkpreyunk requested a review from himself65May 31, 2020 17:35
himself65
himself65 reviewed Jun 1, 2020
View reviewed changes
This was referenced Jun 22, 2020
Open
Open
@preyunk
Copy link
ContributorAuthor

preyunk commented Jun 30, 2020

@himself65 It's been more than 7 days since it was approved, just wanted to know if it could be merged with 1 approval now?

@himself65
Copy link
Member

himself65 commented Jun 30, 2020

Don’t worry. I think we need review by other member to make sure the PR is correct and better.

@preyunkpreyunk requested review from a team as code ownersAugust 10, 2020 16:07
mcollina
mcollina approved these changes Aug 10, 2020
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@preyunkpreyunk requested review from rexagod and removed request for a teamAugust 11, 2020 09:12
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 11, 2020

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

@mcollina
Copy link
Member

mcollina commented Aug 11, 2020

@rexagod it seems your request for change has been addressed. Can you confirm?

@rexagod
Copy link
Member

rexagod commented Aug 11, 2020

It seems this caught a flaky test: https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1015/lastFailedBuild/testReport/junit/(root)/test/parallel_test_inspector_multisession_ws/

I'll open an issue.

@rexagodrexagod mentioned this pull request Aug 11, 2020
Closed
@mcollina
Copy link
Member

mcollina commented Aug 12, 2020

Landed in 3b84048

mcollina pushed a commit that referenced this pull request Aug 12, 2020
@preyunk@mcollina
http2: add maxHeaderSize option to http2
3b84048
add maxHeaderSize to http2 as an alias for maxHeaderListSize. Fixes: #33517 PR-URL: #33636 Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This was referenced Aug 15, 2020
Open
Open
Open
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
@preyunk@MylesBorins
http2: add maxHeaderSize option to http2
ef356f6
add maxHeaderSize to http2 as an alias for maxHeaderListSize. Fixes: #33517 PR-URL: #33636 Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Aug 18, 2020
Open
5 tasks
@danielleadamsdanielleadams mentioned this pull request Aug 20, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@preyunk@BethGriggs
http2: add maxHeaderSize option to http2
6ee2578
add maxHeaderSize to http2 as an alias for maxHeaderListSize. Fixes: #33517 PR-URL: #33636 Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@preyunk@addaleax
http2: add maxHeaderSize option to http2
205afdf
add maxHeaderSize to http2 as an alias for maxHeaderListSize. Fixes: #33517 PR-URL: #33636 Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@preyunk@addaleax
http2: add maxHeaderSize option to http2
45d712c
add maxHeaderSize to http2 as an alias for maxHeaderListSize. Fixes: #33517 PR-URL: #33636 Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Pranshu Srivastava <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@codebyterecodebytere mentioned this pull request Sep 28, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@himself65himself65himself65 approved these changes

+1 more reviewer

@rexagodrexagodrexagod approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2Issues or PRs related to the http2 subsystem.review wantedPRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add 'maxHeaderSize' to 'http2'

5 participants

@preyunk@himself65@rexagod@nodejs-github-bot@mcollina