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.

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

https: defines maxHeadersCount in the constructor #20359

Closed
wants to merge 1 commit into from

Conversation

darai0512
Copy link
Contributor

In Refs, http.Server's maxHeadersCount field was defined in the constructor to make hidden class stable and so on.
Also in https.Server, we can use maxHeadersCount the same as http via connectionListener.
So defines it in the constructor and documentation.

Refs: #9116

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the https Issues or PRs related to the https subsystem. label Apr 27, 2018
@BridgeAR
Copy link
Member

@nodejs/http @nodejs/http2 PTAL


process.on('exit', () => {
assert.strictEqual(requests, maxAndExpected.length);
assert.strictEqual(responses, maxAndExpected.length);
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed if you use common.mustCall().

let max = maxAndExpected[requests][0];
let expected = maxAndExpected[requests][1];

const server = https.createServer(serverOptions, (req, res) => {
Copy link
Member

Choose a reason for hiding this comment

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

the callback should be wrapped in common.mustCall().

];
doRequest();

function doRequest() {
Copy link
Member

Choose a reason for hiding this comment

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

this function should be wrapped in common.mustCall(), with the total number of time it needs to be called.

In Refs, http.Server's maxHeadersCount field was defined in the
constructor to make hidden class stable and so on. Also in https.Server,
we can use maxHeadersCount the same as http via connectionListener. So,
defines it in the constructor and documentation.

Refs: nodejs#9116
@darai0512
Copy link
Contributor Author

@mcollina Thanks for your reviews. PTAL

trivikr pushed a commit that referenced this pull request May 3, 2018
In http.ClientRequest's doc, add maxHeadersCount as a public property.
And in the description of server's one, change a hyphen to a comma.

PR-URL: #20361
Refs: #20359
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 3, 2018
@mcollina
Copy link
Member

mcollina commented May 3, 2018

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

MylesBorins pushed a commit that referenced this pull request May 4, 2018
In http.ClientRequest's doc, add maxHeadersCount as a public property.
And in the description of server's one, change a hyphen to a comma.

PR-URL: #20361
Refs: #20359
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member

addaleax commented May 5, 2018

Landed in 6779096

@addaleax addaleax closed this May 5, 2018
addaleax pushed a commit that referenced this pull request May 5, 2018
In Refs, http.Server's maxHeadersCount field was defined in the
constructor to make hidden class stable and so on. Also in https.Server,
we can use maxHeadersCount the same as http via connectionListener. So,
defines it in the constructor and documentation.

Refs: #9116

PR-URL: #20359
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
In Refs, http.Server's maxHeadersCount field was defined in the
constructor to make hidden class stable and so on. Also in https.Server,
we can use maxHeadersCount the same as http via connectionListener. So,
defines it in the constructor and documentation.

Refs: #9116

PR-URL: #20359
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
MylesBorins pushed a commit that referenced this pull request May 8, 2018
In http.ClientRequest's doc, add maxHeadersCount as a public property.
And in the description of server's one, change a hyphen to a comma.

PR-URL: #20361
Refs: #20359
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
In Refs, http.Server's maxHeadersCount field was defined in the
constructor to make hidden class stable and so on. Also in https.Server,
we can use maxHeadersCount the same as http via connectionListener. So,
defines it in the constructor and documentation.

Refs: #9116

PR-URL: #20359
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
In Refs, http.Server's maxHeadersCount field was defined in the
constructor to make hidden class stable and so on. Also in https.Server,
we can use maxHeadersCount the same as http via connectionListener. So,
defines it in the constructor and documentation.

Refs: #9116

PR-URL: #20359
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants