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

http: refactor headersTimeout and requestTimeout logic #41263

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions benchmark/http/chunked.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@ function main({ len, n, c, duration }) {
send(n);
});

server.listen(common.PORT, () => {
server.listen(0, () => {
bench.http({
connections: c,
duration
duration,
port: server.address().port,
}, () => {
server.close();
});
Expand Down
10 changes: 5 additions & 5 deletions benchmark/http/client-request-body.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,24 @@ function main({ dur, len, type, method }) {
headers: { 'Connection': 'keep-alive', 'Transfer-Encoding': 'chunked' },
agent: new http.Agent({ maxSockets: 1 }),
host: '127.0.0.1',
port: common.PORT,
path: '/',
method: 'POST'
};

const server = http.createServer((req, res) => {
res.end();
});
server.listen(options.port, options.host, () => {
server.listen(0, options.host, () => {
setTimeout(done, dur * 1000);
bench.start();
pummel();
pummel(server.address().port);
});

function pummel() {
function pummel(port) {
options.port = port;
const req = http.request(options, (res) => {
nreqs++;
pummel(); // Line up next request.
pummel(port); // Line up next request.
res.resume();
});
if (method === 'write') {
Expand Down
5 changes: 3 additions & 2 deletions benchmark/http/end-vs-write-end.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@ function main({ len, type, method, c, duration }) {
fn(res);
});

server.listen(common.PORT, () => {
server.listen(0, () => {
bench.http({
connections: c,
duration
duration,
port: server.address().port,
}, () => {
server.close();
});
Expand Down
5 changes: 3 additions & 2 deletions benchmark/http/headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ function main({ len, n, duration }) {
res.writeHead(200, headers);
res.end();
});
server.listen(common.PORT, () => {
server.listen(0, () => {
bench.http({
path: '/',
connections: 10,
duration
duration,
port: server.address().port,
}, () => {
server.close();
});
Expand Down
5 changes: 3 additions & 2 deletions benchmark/http/incoming_headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ function main({ connections, headers, w, duration }) {
res.end();
});

server.listen(common.PORT, () => {
server.listen(0, () => {
const headers = {
'Content-Type': 'text/plain',
'Accept': 'text/plain',
Expand All @@ -34,7 +34,8 @@ function main({ connections, headers, w, duration }) {
path: '/',
connections,
headers,
duration
duration,
port: server.address().port,
}, () => {
server.close();
});
Expand Down
7 changes: 3 additions & 4 deletions benchmark/http/set-header.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict';
const common = require('../common.js');
const PORT = common.PORT;

const bench = common.createBenchmark(main, {
res: ['normal', 'setHeader', 'setHeaderWH'],
Expand All @@ -17,16 +16,16 @@ const c = 50;
// setHeader: statusCode = status, setHeader(...) x2
// setHeaderWH: setHeader(...), writeHead(status, ...)
function main({ res, duration }) {
process.env.PORT = PORT;
const server = require('../fixtures/simple-http-server.js')
.listen(PORT)
.listen(0)
.on('listening', () => {
const path = `/${type}/${len}/${chunks}/${res}/${chunkedEnc}`;

bench.http({
path: path,
connections: c,
duration
duration,
port: server.address().port,
}, () => {
server.close();
});
Expand Down
5 changes: 3 additions & 2 deletions benchmark/http/simple.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@ const bench = common.createBenchmark(main, {

function main({ type, len, chunks, c, chunkedEnc, duration }) {
const server = require('../fixtures/simple-http-server.js')
.listen(common.PORT)
.listen(0)
.on('listening', () => {
const path = `/${type}/${len}/${chunks}/normal/${chunkedEnc}`;

bench.http({
path,
connections: c,
duration
duration,
port: server.address().port,
}, () => {
server.close();
});
Expand Down
2 changes: 1 addition & 1 deletion benchmark/http/upgrade.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const resData = 'HTTP/1.1 101 Web Socket Protocol Handshake\r\n' +

function main({ n }) {
const server = require('../fixtures/simple-http-server.js')
.listen(common.PORT)
.listen(0)
.on('listening', () => {
bench.start();
doBench(server.address(), n, () => {
Expand Down
45 changes: 35 additions & 10 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -1364,15 +1364,12 @@ added:
Limit the amount of time the parser will wait to receive the complete HTTP
headers.

In case of inactivity, the rules defined in [`server.timeout`][] apply. However,
that inactivity based timeout would still allow the connection to be kept open
if the headers are being sent very slowly (by default, up to a byte per 2
minutes). In order to prevent this, whenever header data arrives an additional
check is made that more than `server.headersTimeout` milliseconds has not
passed since the connection was established. If the check fails, a `'timeout'`
event is emitted on the server object, and (by default) the socket is destroyed.
See [`server.timeout`][] for more information on how timeout behavior can be
customized.
If the timeout expires, the server responds with status 408 without
forwarding the request to the request listener and then closes the connection.

It must be set to a non-zero value (e.g. 120 seconds) to protect against
potential Denial-of-Service attacks in case the server is deployed without a
reverse proxy in front.

### `server.listen()`

Expand Down Expand Up @@ -1401,9 +1398,14 @@ Limits maximum incoming headers count. If set to 0, no limit will be applied.

<!-- YAML
added: v14.11.0
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).
-->

* {number} **Default:** `0`
* {number} **Default:** `300000`
ShogunPanda marked this conversation as resolved.
Show resolved Hide resolved
ronag marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey! I believe this change should be added to https.md as well 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@julien-f Yes, it should. Are you willing to send a PR about this or will I do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do it, I don't have the time at the moment.

Thank you 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok!


Sets the timeout value in milliseconds for receiving the entire request from
the client.
Expand Down Expand Up @@ -2856,6 +2858,10 @@ Found'`.
<!-- YAML
added: v0.1.13
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/41263
description: The `requestTimeout`, `headersTimeout`, `keepAliveTimeout` and
`connectionsCheckingInterval` are supported now.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42163
description: The `noDelay` option now defaults to `true`.
Expand Down Expand Up @@ -2886,6 +2892,22 @@ changes:
* `ServerResponse` {http.ServerResponse} Specifies the `ServerResponse` class
to be used. Useful for extending the original `ServerResponse`. **Default:**
`ServerResponse`.
* `requestTimeout`: Sets the timeout value in milliseconds for receiving
ShogunPanda marked this conversation as resolved.
Show resolved Hide resolved
the entire request from the client.
See [`server.requestTimeout`][] for more information.
**Default:** `300000`.
* `headersTimeout`: Sets the timeout value in milliseconds for receiving
the complete HTTP headers from the client.
See [`server.headersTimeout`][] for more information.
**Default:** `60000`.
* `keepAliveTimeout`: The number of milliseconds of inactivity a server
needs to wait for additional incoming data, after it has finished writing
the last response, before a socket will be destroyed.
See [`server.keepAliveTimeout`][] for more information.
**Default:** `5000`.
* `connectionsCheckingInterval`: Sets the interval value in milliseconds to
check for request and headers timeout in incomplete requests.
ShogunPanda marked this conversation as resolved.
Show resolved Hide resolved
**Default:** `30000`.
* `insecureHTTPParser` {boolean} Use an insecure HTTP parser that accepts
invalid HTTP headers when `true`. Using the insecure parser should be
avoided. See [`--insecure-http-parser`][] for more information.
Expand Down Expand Up @@ -3478,7 +3500,10 @@ try {
[`response.write(data, encoding)`]: #responsewritechunk-encoding-callback
[`response.writeContinue()`]: #responsewritecontinue
[`response.writeHead()`]: #responsewriteheadstatuscode-statusmessage-headers
[`server.headersTimeout`]: #serverheaderstimeout
[`server.keepAliveTimeout`]: #serverkeepalivetimeout
[`server.listen()`]: net.md#serverlisten
[`server.requestTimeout`]: #serverrequesttimeout
[`server.timeout`]: #servertimeout
[`setHeader(name, value)`]: #requestsetheadername-value
[`socket.connect()`]: net.md#socketconnectoptions-connectlistener
Expand Down
3 changes: 1 addition & 2 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -735,8 +735,7 @@ function tickOnSocket(req, socket) {
parser.initialize(HTTPParser.RESPONSE,
new HTTPClientAsyncResource('HTTPINCOMINGMESSAGE', req),
req.maxHeaderSize || 0,
lenient ? kLenientAll : kLenientNone,
0);
lenient ? kLenientAll : kLenientNone);
parser.socket = socket;
parser.outgoing = req;
req.parser = parser;
Expand Down
12 changes: 0 additions & 12 deletions lib/_http_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,7 @@ const {
readStop
} = incoming;

let debug = require('internal/util/debuglog').debuglog('http', (fn) => {
debug = fn;
});

const kIncomingMessage = Symbol('IncomingMessage');
const kRequestTimeout = Symbol('RequestTimeout');
const kOnMessageBegin = HTTPParser.kOnMessageBegin | 0;
const kOnHeaders = HTTPParser.kOnHeaders | 0;
const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0;
Expand Down Expand Up @@ -102,12 +97,6 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
incoming.url = url;
incoming.upgrade = upgrade;

if (socket) {
debug('requestTimeout timer moved to req');
incoming[kRequestTimeout] = incoming.socket[kRequestTimeout];
incoming.socket[kRequestTimeout] = undefined;
}

let n = headers.length;

// If parser.maxHeaderPairs <= 0 assume that there's no limit.
Expand Down Expand Up @@ -273,7 +262,6 @@ module.exports = {
methods,
parsers,
kIncomingMessage,
kRequestTimeout,
HTTPParser,
isLenient,
prepareError,
Expand Down