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

http: fix error message when specifying headerTimeout for createServer #44163

Merged
merged 1 commit into from
Aug 9, 2022
Merged

http: fix error message when specifying headerTimeout for createServer #44163

merged 1 commit into from
Aug 9, 2022

Conversation

nicksia-vgw
Copy link
Contributor

@nicksia-vgw nicksia-vgw commented Aug 7, 2022 •
edited by VoltrexKeyva
Loading

This change fixes a misleading error message provided by the error thrown when creating an http server with a headerTimeout that exceeds the requestTimeout.

Code

const server = createServer({
  headersTimeout: 6000, // Limit the amount of time the parser will wait to receive the complete HTTP headers.
  requestTimeout: 3000, // Sets the timeout value in milliseconds for receiving the entire request from the client.
}, () => {});

Error

node:_http_server:406
    throw new codes.ERR_OUT_OF_RANGE('headersTimeout', '>= requestTimeout', headersTimeout);
    ^

RangeError [ERR_OUT_OF_RANGE]: The value of "headersTimeout" is out of range. It must be >= requestTimeout. Received 6000

The header timeout is actually required to be < the request timeout (which is correctly validated), but the error message is wrong - see original snippet below.

_http_server.js.storeHTTPOptions (validation code)

  if (this.requestTimeout > 0 && this.headersTimeout > 0 && this.headersTimeout >= this.requestTimeout) {
    throw new codes.ERR_OUT_OF_RANGE('headersTimeout', '>= requestTimeout', headersTimeout);
  }

@nicksia-vgw
http: fix error message when specifying headerTimeout for createServer
9dde596 
This change fixes the message on the error received when calling 
http.createServer(...) with a header timeout greater than the request
timeout is incorrect.

The validation requires that the header timeout is lower than the 
request timeout, but the error message asks for the opposite.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Aug 7, 2022
ShogunPanda
ShogunPanda approved these changes Aug 7, 2022
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda 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 approved these changes Aug 7, 2022
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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Aug 8, 2022
Open
11 tasks
@github-actions github-actions bot mentioned this pull request Aug 9, 2022
Open
8 tasks
@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Aug 9, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 9, 2022
@nodejs-github-bot nodejs-github-bot merged commit b427924 into nodejs:main Aug 9, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in b427924

@nicksia-vgw nicksia-vgw deleted the fix-incorrect-headertimeout-error-message branch August 9, 2022 09:33
This was referenced Aug 10, 2022
Open
Open
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
@nicksia-vgw @danielleadams
http: fix error message when specifying headerTimeout for createServer
f5fcd49 
This change fixes the message on the error received when calling
http.createServer(...) with a header timeout greater than the request
timeout is incorrect.

The validation requires that the header timeout is lower than the
request timeout, but the error message asks for the opposite.

PR-URL: #44163
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@danielleadams danielleadams mentioned this pull request Aug 16, 2022
Closed
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
@nicksia-vgw @ruyadorno
http: fix error message when specifying headerTimeout for createServer
340ca4d 
This change fixes the message on the error received when calling
http.createServer(...) with a header timeout greater than the request
timeout is incorrect.

The validation requires that the header timeout is lower than the
request timeout, but the error message asks for the opposite.

PR-URL: #44163
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Aug 23, 2022
Merged
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@nicksia-vgw @Fyko
http: fix error message when specifying headerTimeout for createServer
fdc7e57 
This change fixes the message on the error received when calling
http.createServer(...) with a header timeout greater than the request
timeout is incorrect.

The validation requires that the header timeout is lower than the
request timeout, but the error message asks for the opposite.

PR-URL: nodejs#44163
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@juanarbol
Copy link
Member

This change depends on #41263 which is a semver-major PR

@ShogunPanda ShogunPanda mentioned this pull request Dec 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickyes rickyes rickyes approved these changes

@ErickWendel ErickWendel ErickWendel approved these changes

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nicksia-vgw @nodejs-github-bot @juanarbol @mcollina @ShogunPanda @lpinca @tniessen @ErickWendel @rickyes