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

http2: add maxHeaderSize option to http2 #33636

Closed
wants to merge 7 commits into from
Closed

http2: add maxHeaderSize option to http2 #33636

wants to merge 7 commits into from

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
lib/internal/http2/util.js Outdated Show resolved Hide resolved
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.

lib/internal/http2/util.js Outdated Show resolved Hide resolved
lib/internal/http2/util.js Outdated Show resolved Hide resolved
@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
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
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 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
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

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

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

@mcollina
Copy link
Member

@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

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

@rexagod rexagod rexagod approved these changes

@himself65 himself65 himself65 approved these changes

@mcollina mcollina mcollina approved these changes

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