Skip to content

http2: add maxHeaderSize option to http2#33636

Closed
preyunk wants to merge 7 commits intonodejs:masterfrom
preyunk:http2-max-header
Closed

http2: add maxHeaderSize option to http2#33636
preyunk wants to merge 7 commits intonodejs:masterfrom
preyunk:http2-max-header

Conversation

@preyunk
Copy link
Contributor

@preyunk preyunk 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-bot nodejs-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

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

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

@rexagod rexagod 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 preyunk force-pushed the http2-max-header branch from 96dd578 to 205f38a Compare May 29, 2020 21:36
@preyunk
Copy link
Contributor Author

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

@preyunk preyunk force-pushed the http2-max-header branch 2 times, most recently from cc3ffed to 2f3782c Compare May 30, 2020 14:58
@preyunk preyunk requested a review from himself65 May 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

@himself65 himself65 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 preyunk force-pushed the http2-max-header branch from 38a8998 to 5857ca5 Compare May 31, 2020 08:13
@preyunk
Copy link
Contributor Author

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.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 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

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

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.

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

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@preyunk preyunk requested review from rexagod and removed request for a team August 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.

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

mcollina commented Aug 12, 2020

Landed in 3b84048

@mcollina mcollina closed this Aug 12, 2020
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 <himself65@outlook.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <himself65@outlook.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@github-actions github-actions bot mentioned this pull request Aug 18, 2020
Open
5 tasks
@danielleadams danielleadams 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 <himself65@outlook.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <himself65@outlook.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <himself65@outlook.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@himself65 himself65 himself65 approved these changes

+1 more reviewer

@rexagod rexagod rexagod approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem. review wanted PRs 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