Fixed http_server timeout logic for requestTimeout and headerTimeout

[emilfihlman]

According to the documentation and also logic, headersTimeout should be smaller or equal to requestTimeout. This fixes the logic handling this check.

Node could also clamp requestTimeout to headersTimeout if the latter is larger, but that's a design decision for a different
person.

I suspect there might be other range issues like this, too.

Fixes: #43355

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[aduh95]

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?

[emilfihlman]

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

[aduh95]

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?

[emilfihlman]

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.

[ShogunPanda]

I agree with @aduh95. The check on line 394 is correct and needs no fix. Having headersTimeout equals to requestTimeout is redundant.

The fix on line 395 (the error thrown) is instead as the message is wrong (my bad :()

@emilfihlman By documentation do you mean the message thrown?

[emilfihlman]

E: I apologise for the tone on this comment.

@ShogunPanda if it's as intended then the intention should be changed. A request can be complete when the headers arrive, thus it's silly to require that requestTimeout needs to be say 1001 if you want headersTimeout to be 1000.

Imho the code clearly shows that the intention was that headersTimeout<=requestTimeout, and it should be that way.

I'd very much like to hear a justified reason as to why headersTimeout couldn't be requestTimeout and why it needs to be smaller and not selectable to be the same.

Additionally, not allowing headersTimeout to be requestTimeout would result in inability to limit requestHandling to 1ms, always requiring it to be 2ms at minimum. Sure, it's a small edge case, but I can definitely see it happening at some point, and we shouldn't prevent people from making small timeouts if they so wish.

[aduh95]

@ShogunPanda it does seem a sub-optimal API design that setting requestTimeout to a value lower than 60000 forces the user to also set a value for headersTimeout (which can't be the same). Should we change headersTimeout default value to be Math.min(60_000, requestTimeout)?

[ShogunPanda]

First of all, headersTimeout can definitely be equal as requestTimeout. It's harmless as the connections checking interval will anyway detect connection as expired. Just to clarify, now that requestTimeout exists and works properly regardless of the presence of the body, headersTimeout is not needed most of the times.

Anyway, @emilfihlman I suggest to do the following on this PR:

1. Allow headersTimeout === requestTimeout
2. Set default value of headersTimeout as Math.min(60_000, requestTimeout) as @aduh95 suggested and fix the relevant documentation
3. Fix the typo in the error message.

Are you willing to perform the changes this in PR?

[emilfihlman]

@ShogunPanda sure!

[ShogunPanda]

@emilfihlman Are you willing to update this or shall we close this PR and continue on our own?

[ShogunPanda]

This has been superseded by #44163 and #45778.
Closing.