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

Fixed http_server timeout logic for requestTimeout and headerTimeout #43354

Closed
wants to merge 1 commit into from
Closed

Fixed http_server timeout logic for requestTimeout and headerTimeout #43354

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,8 +391,8 @@ function storeHTTPOptions(options) {
this.headersTimeout = 60_000; // 60 seconds
}

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

Choose a reason for hiding this comment

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

What's the reasoning for changing this check? Before this PR, it was not accepted to have headersTimeout equals to requestTimeout. You mentioned the docs in the OP, but I don't seem to find that; could you link to where you've read that?

Copy link
Author

Choose a reason for hiding this comment

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

@aduh95 a request can be complete when the headers have been finished. Looking at the code and the docs for headersTimeout and requestTimeout, it's imho clear that the intention is/was that headersTimeout<=requestTimeout. Additionally people like to be able to set their arguments logically, so requiring that requestTimeout needs to be 1001 if you want headersTimeout to be 1000 is silly.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said in my previous comment, I wasn't able to find that in the docs. When referring to the docs, it'd be helpful to quote the exact docs so we know we're talking of the same thing (or even better use the permalink feature of GitHub). (and if it is not in the docs, that's also fine, but in this case you shouldn't say "According to the documentation and also logic", but rather "According to me": that'd still be a valid reason to make that change and reviewers wouldn't waste time reading the docs looking for something that's not there 😉)

Since it is a behavior change, we should add an history entry to the YAML comment. That would look something like this (but with a different pr-url & description of course):

node/doc/api/http.md

Lines 1401 to 1405 in 3caa2c1

changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/41263
description: The default request timeout changed
from no timeout to 300s (5 minutes).

it's imho clear that the intention is/was that headersTimeout<=requestTimeout

Let's not assume intent, I don't think that's relevant, and it's easy to get wrong.

requiring that requestTimeout needs to be 1001 if you want headersTimeout to be 1000 is silly.

I agree that does seem to be an unnecessary requirement – unless I'm missing something? @ShogunPanda when you said it's redundant, do you mean that users should just remove headersTimeout if it's set to the same value as requestTimeout?

Copy link
Author

Choose a reason for hiding this comment

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

Apologies, I was arrogant. I appreciate you being kind to me.

Yes, the docs don't cleanly say that it must be x. I take it as implied, since the docs say:

For headersTimeout
"Limit the amount of time the parser will wait to receive the complete HTTP headers."

For requestTimeout
"Sets the timeout value in milliseconds for receiving the entire request from the client."

and a complete entire request can be just the http headers.

but implication is also easy to get wrong.

I wish it was so, that we could cleanly set headersTimeout as <= of requestTimeout. It allows explicitly defining what happens, and would, in my opinion, be logical.

throw new codes.ERR_OUT_OF_RANGE('headersTimeout', '<= requestTimeout', headersTimeout);
}

const keepAliveTimeout = options.keepAliveTimeout;
Expand Down