diff options
author | Paolo Insogna <paolo@cowtech.it> | 2022-04-13 17:47:59 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-04-13 17:47:59 +0300 |
commit | 3caa2c1a005652fdb3e896ef940cd5ffe5fdff10 (patch) | |
tree | f8e9e9bfe7c95d5633c86e021518f487a9eadb8f | |
parent | 9d6af7d1fe66afdcb781fb5bad37b4cb4d396f0e (diff) |
http: refactor headersTimeout and requestTimeout logic
PR-URL: https://github.com/nodejs/node/pull/41263
Fixes: https://github.com/nodejs/node/issues/33440
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
33 files changed, 858 insertions, 413 deletions
diff --git a/benchmark/http/chunked.js b/benchmark/http/chunked.js index 9ae7bb7495f..ec86d0a1f65 100644 --- a/benchmark/http/chunked.js +++ b/benchmark/http/chunked.js @@ -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(); }); diff --git a/benchmark/http/client-request-body.js b/benchmark/http/client-request-body.js index 6e2323b3f07..3d673d36b7b 100644 --- a/benchmark/http/client-request-body.js +++ b/benchmark/http/client-request-body.js @@ -32,7 +32,6 @@ 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' }; @@ -40,16 +39,17 @@ function main({ dur, len, type, method }) { 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') { diff --git a/benchmark/http/end-vs-write-end.js b/benchmark/http/end-vs-write-end.js index 60174ef3adf..9f128bf8c14 100644 --- a/benchmark/http/end-vs-write-end.js +++ b/benchmark/http/end-vs-write-end.js @@ -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(); }); diff --git a/benchmark/http/headers.js b/benchmark/http/headers.js index a3d2b7810be..b405868eb0e 100644 --- a/benchmark/http/headers.js +++ b/benchmark/http/headers.js @@ -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(); }); diff --git a/benchmark/http/incoming_headers.js b/benchmark/http/incoming_headers.js index 983bd5632fc..7501d2cb92d 100644 --- a/benchmark/http/incoming_headers.js +++ b/benchmark/http/incoming_headers.js @@ -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', @@ -34,7 +34,8 @@ function main({ connections, headers, w, duration }) { path: '/', connections, headers, - duration + duration, + port: server.address().port, }, () => { server.close(); }); diff --git a/benchmark/http/set-header.js b/benchmark/http/set-header.js index 48e0163a6ce..47681ebd1b1 100644 --- a/benchmark/http/set-header.js +++ b/benchmark/http/set-header.js @@ -1,6 +1,5 @@ 'use strict'; const common = require('../common.js'); -const PORT = common.PORT; const bench = common.createBenchmark(main, { res: ['normal', 'setHeader', 'setHeaderWH'], @@ -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(); }); diff --git a/benchmark/http/simple.js b/benchmark/http/simple.js index 095b15ca446..31e12ca7c27 100644 --- a/benchmark/http/simple.js +++ b/benchmark/http/simple.js @@ -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(); }); diff --git a/benchmark/http/upgrade.js b/benchmark/http/upgrade.js index 8d365fe46df..c4f5cd34228 100644 --- a/benchmark/http/upgrade.js +++ b/benchmark/http/upgrade.js @@ -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, () => { diff --git a/doc/api/http.md b/doc/api/http.md index 5c3c7d9a7a7..851679dfc31 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -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()` @@ -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` Sets the timeout value in milliseconds for receiving the entire request from the client. @@ -2857,6 +2859,10 @@ Found'`. 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`. - version: v17.7.0 @@ -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 + 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. + **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. @@ -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 diff --git a/lib/_http_client.js b/lib/_http_client.js index 42b4683d141..a4f7a255a99 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -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; diff --git a/lib/_http_common.js b/lib/_http_common.js index 796deeff055..2055ca205b8 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -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; @@ -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. @@ -273,7 +262,6 @@ module.exports = { methods, parsers, kIncomingMessage, - kRequestTimeout, HTTPParser, isLenient, prepareError, diff --git a/lib/_http_server.js b/lib/_http_server.js index 00c810e51df..84be32c78c4 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -27,6 +27,7 @@ const { ObjectKeys, ObjectSetPrototypeOf, RegExpPrototypeTest, + ReflectApply, Symbol, SymbolFor, } = primordials; @@ -40,12 +41,12 @@ const { continueExpression, chunkExpression, kIncomingMessage, - kRequestTimeout, HTTPParser, isLenient, _checkInvalidHeaderChar: checkInvalidHeaderChar, prepareError, } = require('_http_common'); +const { ConnectionsList } = internalBinding('http_parser'); const { OutgoingMessage } = require('_http_outgoing'); const { kOutHeaders, @@ -79,8 +80,7 @@ const { DTRACE_HTTP_SERVER_REQUEST, DTRACE_HTTP_SERVER_RESPONSE } = require('internal/dtrace'); -const { setTimeout, clearTimeout } = require('timers'); - +const { setInterval, clearInterval } = require('timers'); let debug = require('internal/util/debuglog').debuglog('http', (fn) => { debug = fn; }); @@ -162,11 +162,12 @@ const STATUS_CODES = { 511: 'Network Authentication Required' // RFC 6585 6 }; -const kOnMessageBegin = HTTPParser.kOnMessageBegin | 0; const kOnExecute = HTTPParser.kOnExecute | 0; const kOnTimeout = HTTPParser.kOnTimeout | 0; const kLenientAll = HTTPParser.kLenientAll | 0; const kLenientNone = HTTPParser.kLenientNone | 0; +const kConnections = Symbol('http.server.connections'); +const kConnectionsCheckingInterval = Symbol('http.server.connectionsCheckingInterval'); class HTTPServerAsyncResource { constructor(type, socket) { @@ -367,6 +368,51 @@ function storeHTTPOptions(options) { if (options.noDelay === undefined) options.noDelay = true; + + const requestTimeout = options.requestTimeout; + if (requestTimeout !== undefined) { + validateInteger(requestTimeout, 'requestTimeout', 0); + this.requestTimeout = requestTimeout; + } else { + this.requestTimeout = 300_000; // 5 minutes + } + + const headersTimeout = options.headersTimeout; + if (headersTimeout !== undefined) { + validateInteger(headersTimeout, 'headersTimeout', 0); + this.headersTimeout = headersTimeout; + } else { + 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); + } + + const keepAliveTimeout = options.keepAliveTimeout; + if (keepAliveTimeout !== undefined) { + validateInteger(keepAliveTimeout, 'keepAliveTimeout', 0); + this.keepAliveTimeout = keepAliveTimeout; + } else { + this.keepAliveTimeout = 5_000; // 5 seconds; + } + + const connectionsCheckingInterval = options.connectionsCheckingInterval; + if (connectionsCheckingInterval !== undefined) { + validateInteger(connectionsCheckingInterval, 'connectionsCheckingInterval', 0); + this.connectionsCheckingInterval = connectionsCheckingInterval; + } else { + this.connectionsCheckingInterval = 30_000; // 30 seconds + } +} + +function setupConnectionsTracking(server) { + // Start connection handling + server[kConnections] = new ConnectionsList(); + if (server.headersTimeout > 0 || server.requestTimeout > 0) { + server[kConnectionsCheckingInterval] = + setInterval(checkConnections.bind(server), server.connectionsCheckingInterval).unref(); + } } function Server(options, requestListener) { @@ -400,15 +446,17 @@ function Server(options, requestListener) { this.on('connection', connectionListener); this.timeout = 0; - this.keepAliveTimeout = 5000; this.maxHeadersCount = null; this.maxRequestsPerSocket = 0; - this.headersTimeout = 60 * 1000; // 60 seconds - this.requestTimeout = 0; + setupConnectionsTracking(this); } ObjectSetPrototypeOf(Server.prototype, net.Server.prototype); ObjectSetPrototypeOf(Server, net.Server); +Server.prototype.close = function() { + clearInterval(this[kConnectionsCheckingInterval]); + ReflectApply(net.Server.prototype.close, this, arguments); +}; Server.prototype.setTimeout = function setTimeout(msecs, callback) { this.timeout = msecs; @@ -440,6 +488,18 @@ Server.prototype[EE.captureRejectionSymbol] = function(err, event, ...args) { } }; +function checkConnections() { + const expired = this[kConnections].expired(this.headersTimeout, this.requestTimeout); + + for (let i = 0; i < expired.length; i++) { + const socket = expired[i].socket; + + if (socket) { + onRequestTimeout(socket); + } + } +} + function connectionListener(socket) { defaultTriggerAsyncIdScope( getOrSetAsyncId(socket), connectionListenerInternal, this, socket @@ -473,7 +533,7 @@ function connectionListenerInternal(server, socket) { new HTTPServerAsyncResource('HTTPINCOMINGMESSAGE', socket), server.maxHeaderSize || 0, lenient ? kLenientAll : kLenientNone, - server.headersTimeout || 0, + server[kConnections], ); parser.socket = socket; socket.parser = parser; @@ -539,17 +599,6 @@ function connectionListenerInternal(server, socket) { onParserTimeout.bind(undefined, server, socket); - // When receiving new requests on the same socket (pipelining or keep alive) - // make sure the requestTimeout is active. - parser[kOnMessageBegin] = - setRequestTimeout.bind(undefined, - server, socket); - - // This protects from DOS attack where an attacker establish the connection - // without sending any data on applications where server.timeout is left to - // the default value of zero. - setRequestTimeout(server, socket); - socket._paused = false; } @@ -632,7 +681,6 @@ function socketOnData(server, socket, parser, state, d) { } function onRequestTimeout(socket) { - socket[kRequestTimeout] = undefined; // socketOnError has additional logic and will call socket.destroy(err). socketOnError.call(socket, new ERR_HTTP_REQUEST_TIMEOUT()); } @@ -727,9 +775,6 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) { socket.readableFlowing = null; - // Clear the requestTimeout after upgrading the connection. - clearRequestTimeout(req); - server.emit(eventName, req, socket, bodyHead); } else { // Got CONNECT method, but have no handler. @@ -738,11 +783,6 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) { } else if (parser.incoming && parser.incoming.method === 'PRI') { debug('SERVER got PRI request'); socket.destroy(); - } else { - // When receiving new requests on the same socket (pipelining or keep alive) - // make sure the requestTimeout is active. - parser[kOnMessageBegin] = - setRequestTimeout.bind(undefined, server, socket); } if (socket._paused && socket.parser) { @@ -765,32 +805,6 @@ function clearIncoming(req) { } } -function setRequestTimeout(server, socket) { - // Set the request timeout handler. - if ( - !socket[kRequestTimeout] && - server.requestTimeout && server.requestTimeout > 0 - ) { - debug('requestTimeout timer set'); - socket[kRequestTimeout] = - setTimeout(onRequestTimeout, server.requestTimeout, socket).unref(); - } -} - -function clearRequestTimeout(req) { - if (!req) { - req = this; - } - - if (!req[kRequestTimeout]) { - return; - } - - debug('requestTimeout timer cleared'); - clearTimeout(req[kRequestTimeout]); - req[kRequestTimeout] = undefined; -} - function resOnFinish(req, res, socket, state, server) { if (onResponseFinishChannel.hasSubscribers) { onResponseFinishChannel.publish({ @@ -814,14 +828,6 @@ function resOnFinish(req, res, socket, state, server) { if (!req._consuming && !req._readableState.resumeScheduled) req._dump(); - // Make sure the requestTimeout is cleared before finishing. - // This might occur if the application has sent a response - // without consuming the request body, which would have already - // cleared the timer. - // clearRequestTimeout can be executed even if the timer is not active, - // so this is safe. - clearRequestTimeout(req); - res.detachSocket(socket); clearIncoming(req); process.nextTick(emitCloseNT, res); @@ -955,7 +961,6 @@ function parserOnIncoming(server, socket, state, req, keepAlive) { } if (!handled) { - req.on('end', clearRequestTimeout); server.emit('request', req, res); } @@ -1027,6 +1032,7 @@ module.exports = { STATUS_CODES, Server, ServerResponse, + setupConnectionsTracking, storeHTTPOptions, _connectionListener: connectionListener, kServerResponse diff --git a/lib/https.js b/lib/https.js index 3834a881775..74ce93baaaf 100644 --- a/lib/https.js +++ b/lib/https.js @@ -40,6 +40,7 @@ const tls = require('tls'); const { Agent: HttpAgent } = require('_http_agent'); const { Server: HttpServer, + setupConnectionsTracking, storeHTTPOptions, _connectionListener, } = require('_http_server'); @@ -80,10 +81,8 @@ function Server(opts, requestListener) { }); this.timeout = 0; - this.keepAliveTimeout = 5000; this.maxHeadersCount = null; - this.headersTimeout = 60 * 1000; // 60 seconds - this.requestTimeout = 0; + setupConnectionsTracking(this); } ObjectSetPrototypeOf(Server.prototype, tls.Server.prototype); ObjectSetPrototypeOf(Server, tls.Server); diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index d70e15b8c03..c1255b8cbd3 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -60,6 +60,7 @@ using v8::FunctionTemplate; using v8::HandleScope; using v8::Int32; using v8::Integer; +using v8::Isolate; using v8::Local; using v8::MaybeLocal; using v8::Number; @@ -187,7 +188,58 @@ struct StringPtr { size_t size_; }; +class Parser; + +struct ParserComparator { + bool operator()(const Parser* lhs, const Parser* rhs) const; +}; + +class ConnectionsList : public BaseObject { + public: + static void New(const FunctionCallbackInfo<Value>& args); + + static void All(const FunctionCallbackInfo<Value>& args); + + static void Idle(const FunctionCallbackInfo<Value>& args); + + static void Active(const FunctionCallbackInfo<Value>& args); + + static void Expired(const FunctionCallbackInfo<Value>& args); + + void Push(Parser* parser) { + all_connections_.insert(parser); + } + + void Pop(Parser* parser) { + all_connections_.erase(parser); + } + + void PushActive(Parser* parser) { + active_connections_.insert(parser); + } + + void PopActive(Parser* parser) { + active_connections_.erase(parser); + } + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(ConnectionsList) + SET_SELF_SIZE(ConnectionsList) + + private: + ConnectionsList(Environment* env, Local<Object> object) + : BaseObject(env, object) { + MakeWeak(); + } + + std::set<Parser*, ParserComparator> all_connections_; + std::set<Parser*, ParserComparator> active_connections_; +}; + class Parser : public AsyncWrap, public StreamListener { + friend class ConnectionsList; + friend struct ParserComparator; + public: Parser(BindingData* binding_data, Local<Object> wrap) : AsyncWrap(binding_data->env(), wrap), @@ -205,10 +257,21 @@ class Parser : public AsyncWrap, public StreamListener { SET_SELF_SIZE(Parser) int on_message_begin() { + // Important: Pop from the list BEFORE resetting the last_message_start_ + // otherwise std::set.erase will fail. + if (connectionsList_ != nullptr) { + connectionsList_->PopActive(this); + } + num_fields_ = num_values_ = 0; + headers_completed_ = false; + last_message_start_ = uv_hrtime(); url_.Reset(); status_message_.Reset(); - header_parsing_start_time_ = uv_hrtime(); + + if (connectionsList_ != nullptr) { + connectionsList_->PushActive(this); + } Local<Value> cb = object()->Get(env()->context(), kOnMessageBegin) .ToLocalChecked(); @@ -297,8 +360,8 @@ class Parser : public AsyncWrap, public StreamListener { int on_headers_complete() { + headers_completed_ = true; header_nread_ = 0; - header_parsing_start_time_ = 0; // Arguments for the on-headers-complete javascript callback. This // list needs to be kept in sync with the actual argument list for @@ -429,6 +492,14 @@ class Parser : public AsyncWrap, public StreamListener { int on_message_complete() { HandleScope scope(env()->isolate()); + // Important: Pop from the list BEFORE resetting the last_message_start_ + // otherwise std::set.erase will fail. + if (connectionsList_ != nullptr) { + connectionsList_->PopActive(this); + } + + last_message_start_ = 0; + if (num_fields_) Flush(); // Flush trailing HTTP headers. @@ -486,6 +557,11 @@ class Parser : public AsyncWrap, public StreamListener { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); + if (parser->connectionsList_ != nullptr) { + parser->connectionsList_->Pop(parser); + parser->connectionsList_->PopActive(parser); + } + // Since the Parser destructor isn't going to run the destroy() callbacks // it needs to be triggered manually. parser->EmitTraceEventDestroy(); @@ -506,7 +582,6 @@ class Parser : public AsyncWrap, public StreamListener { } } - // var bytesParsed = parser->execute(buffer); static void Execute(const FunctionCallbackInfo<Value>& args) { Parser* parser; @@ -545,8 +620,8 @@ class Parser : public AsyncWrap, public StreamListener { Environment* env = Environment::GetCurrent(args); uint64_t max_http_header_size = 0; - uint64_t headers_timeout = 0; uint32_t lenient_flags = kLenientNone; + ConnectionsList* connectionsList = nullptr; CHECK(args[0]->IsInt32()); CHECK(args[1]->IsObject()); @@ -565,9 +640,9 @@ class Parser : public AsyncWrap, public StreamListener { lenient_flags = args[3].As<Int32>()->Value(); } - if (args.Length() > 4) { - CHECK(args[4]->IsInt32()); - headers_timeout = args[4].As<Int32>()->Value(); + if (args.Length() > 4 && !args[4]->IsNullOrUndefined()) { + CHECK(args[4]->IsObject()); + ASSIGN_OR_RETURN_UNWRAP(&connectionsList, args[4]); } llhttp_type_t type = @@ -586,7 +661,21 @@ class Parser : public AsyncWrap, public StreamListener { parser->set_provider_type(provider); parser->AsyncReset(args[1].As<Object>()); - parser->Init(type, max_http_header_size, lenient_flags, headers_timeout); + parser->Init(type, max_http_header_size, lenient_flags); + + if (connectionsList != nullptr) { + parser->connectionsList_ = connectionsList; + + parser->connectionsList_->Push(parser); + + // This protects from a DoS attack where an attacker establishes + // the connection without sending any data on applications where + // server.timeout is left to the default value of zero. + parser->last_message_start_ = uv_hrtime(); + parser->connectionsList_->PushActive(parser); + } else { + parser->connectionsList_ = nullptr; + } } template <bool should_pause> @@ -644,6 +733,26 @@ class Parser : public AsyncWrap, public StreamListener { args.GetReturnValue().Set(ret); } + static void Duration(const FunctionCallbackInfo<Value>& args) { + Parser* parser; + ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); + + if (parser->last_message_start_ == 0) { + args.GetReturnValue().Set(0); + return; + } + + double duration = (uv_hrtime() - parser->last_message_start_) / 1e6; + args.GetReturnValue().Set(duration); + } + + static void HeadersCompleted(const FunctionCallbackInfo<Value>& args) { + Parser* parser; + ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); + + args.GetReturnValue().Set(parser->headers_completed_); + } + protected: static const size_t kAllocBufferSize = 64 * 1024; @@ -690,24 +799,6 @@ class Parser : public AsyncWrap, public StreamListener { if (ret.IsEmpty()) return; - // check header parsing time - if (header_parsing_start_time_ != 0 && headers_timeout_ != 0) { - uint64_t now = uv_hrtime(); - uint64_t parsing_time = (now - header_parsing_start_time_) / 1000000; - - if (parsing_time > headers_timeout_) { - Local<Value> cb = - object()->Get(env()->context(), kOnTimeout).ToLocalChecked(); - - if (!cb->IsFunction()) - return; - - MakeCallback(cb.As<Function>(), 0, nullptr); - - return; - } - } - Local<Value> cb = object()->Get(env()->context(), kOnExecute).ToLocalChecked(); @@ -853,7 +944,7 @@ class Parser : public AsyncWrap, public StreamListener { void Init(llhttp_type_t type, uint64_t max_http_header_size, - uint32_t lenient_flags, uint64_t headers_timeout) { + uint32_t lenient_flags) { llhttp_init(&parser_, type, &settings); if (lenient_flags & kLenientHeaders) { @@ -873,9 +964,8 @@ class Parser : public AsyncWrap, public StreamListener { num_values_ = 0; have_flushed_ = false; got_exception_ = false; + headers_completed_ = false; max_http_header_size_ = max_http_header_size; - header_parsing_start_time_ = 0; - headers_timeout_ = headers_timeout; } @@ -923,11 +1013,12 @@ class Parser : public AsyncWrap, public StreamListener { size_t current_buffer_len_; const char* current_buffer_data_; unsigned int execute_depth_ = 0; + bool headers_completed_ = false; bool pending_pause_ = false; uint64_t header_nread_ = 0; uint64_t max_http_header_size_; - uint64_t headers_timeout_; - uint64_t header_parsing_start_time_ = 0; + uint64_t last_message_start_; + ConnectionsList* connectionsList_; BaseObjectPtr<BindingData> binding_data_; @@ -952,6 +1043,135 @@ class Parser : public AsyncWrap, public StreamListener { static const llhttp_settings_t settings; }; +bool ParserComparator::operator()(const Parser* lhs, const Parser* rhs) const { + if (lhs->last_message_start_ == 0) { + return false; + } else if (rhs->last_message_start_ == 0) { + return true; + } + + return lhs->last_message_start_ < rhs->last_message_start_; +} + +void ConnectionsList::New(const FunctionCallbackInfo<Value>& args) { + Local<Context> context = args.GetIsolate()->GetCurrentContext(); + Environment* env = Environment::GetCurrent(context); + + new ConnectionsList(env, args.This()); +} + +void ConnectionsList::All(const FunctionCallbackInfo<Value>& args) { + Isolate* isolate = args.GetIsolate(); + Local<Context> context = isolate->GetCurrentContext(); + + Local<Array> all = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + + uint32_t i = 0; + for (auto parser : list->all_connections_) { + if (all->Set(context, i++, parser->object()).IsNothing()) { + return; + } + } + + return args.GetReturnValue().Set(all); +} + +void ConnectionsList::Idle(const FunctionCallbackInfo<Value>& args) { + Isolate* isolate = args.GetIsolate(); + Local<Context> context = isolate->GetCurrentContext(); + + Local<Array> idle = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + + uint32_t i = 0; + for (auto parser : list->all_connections_) { + if (parser->last_message_start_ == 0) { + if (idle->Set(context, i++, parser->object()).IsNothing()) { + return; + } + } + } + + return args.GetReturnValue().Set(idle); +} + +void ConnectionsList::Active(const FunctionCallbackInfo<Value>& args) { + Isolate* isolate = args.GetIsolate(); + Local<Context> context = isolate->GetCurrentContext(); + + Local<Array> active = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + + uint32_t i = 0; + for (auto parser : list->active_connections_) { + if (active->Set(context, i++, parser->object()).IsNothing()) { + return; + } + } + + return args.GetReturnValue().Set(active); +} + +void ConnectionsList::Expired(const FunctionCallbackInfo<Value>& args) { + Isolate* isolate = args.GetIsolate(); + Local<Context> context = isolate->GetCurrentContext(); + + Local<Array> expired = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + CHECK(args[0]->IsNumber()); + CHECK(args[1]->IsNumber()); + uint64_t headers_timeout = + static_cast<uint64_t>(args[0].As<Uint32>()->Value()) * 1000000; + uint64_t request_timeout = + static_cast<uint64_t>(args[1].As<Uint32>()->Value()) * 1000000; + + if (headers_timeout == 0 && request_timeout == 0) { + return args.GetReturnValue().Set(expired); + } else if (request_timeout > 0 && headers_timeout > request_timeout) { + std::swap(headers_timeout, request_timeout); + } + + const uint64_t now = uv_hrtime(); + const uint64_t headers_deadline = + headers_timeout > 0 ? now - headers_timeout : 0; + const uint64_t request_deadline = + request_timeout > 0 ? now - request_timeout : 0; + + uint32_t i = 0; + auto iter = list->active_connections_.begin(); + auto end = list->active_connections_.end(); + while (iter != end) { + Parser* parser = *iter; + iter++; + + // Check for expiration. + if ( + (!parser->headers_completed_ && headers_deadline > 0 && + parser->last_message_start_ < headers_deadline) || + ( + request_deadline > 0 && + parser->last_message_start_ < request_deadline) + ) { + if (expired->Set(context, i++, parser->object()).IsNothing()) { + return; + } + + list->active_connections_.erase(parser); + } + } + + return args.GetReturnValue().Set(expired); +} + const llhttp_settings_t Parser::settings = { Proxy<Call, &Parser::on_message_begin>::Raw, Proxy<DataCall, &Parser::on_url>::Raw, @@ -1038,8 +1258,19 @@ void InitializeHttpParser(Local<Object> target, env->SetProtoMethod(t, "consume", Parser::Consume); env->SetProtoMethod(t, "unconsume", Parser::Unconsume); env->SetProtoMethod(t, "getCurrentBuffer", Parser::GetCurrentBuffer); + env->SetProtoMethod(t, "duration", Parser::Duration); + env->SetProtoMethod(t, "headersCompleted", Parser::HeadersCompleted); env->SetConstructorFunction(target, "HTTPParser", t); + + Local<FunctionTemplate> c = env->NewFunctionTemplate(ConnectionsList::New); + c->InstanceTemplate() + ->SetInternalFieldCount(ConnectionsList::kInternalFieldCount); + env->SetProtoMethod(c, "all", ConnectionsList::All); + env->SetProtoMethod(c, "idle", ConnectionsList::Idle); + env->SetProtoMethod(c, "active", ConnectionsList::Active); + env->SetProtoMethod(c, "expired", ConnectionsList::Expired); + env->SetConstructorFunction(target, "ConnectionsList", c); } } // anonymous namespace diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js index 3e36646e54e..0a003427f9e 100644 --- a/test/async-hooks/test-graph.http.js +++ b/test/async-hooks/test-graph.http.js @@ -44,7 +44,7 @@ process.on('exit', () => { triggerAsyncId: 'tcp:2' }, { type: 'Timeout', id: 'timeout:1', - triggerAsyncId: 'httpincomingmessage:1' }, + triggerAsyncId: null }, { type: 'SHUTDOWNWRAP', id: 'shutdown:1', triggerAsyncId: 'tcp:2' } ] diff --git a/test/parallel/test-http-parser-timeout-reset.js b/test/parallel/test-http-parser-timeout-reset.js index ffc3d81c381..c51daffaafb 100644 --- a/test/parallel/test-http-parser-timeout-reset.js +++ b/test/parallel/test-http-parser-timeout-reset.js @@ -27,7 +27,6 @@ const server = net.createServer((socket) => { {}, 0, 0, - 1e3 ); parser[HTTPParser.kOnTimeout] = common.mustNotCall(); diff --git a/test/parallel/test-http-server-headers-timeout-delayed-headers.js b/test/parallel/test-http-server-headers-timeout-delayed-headers.js new file mode 100644 index 00000000000..5c184923849 --- /dev/null +++ b/test/parallel/test-http-server-headers-timeout-delayed-headers.js @@ -0,0 +1,61 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { createServer } = require('http'); +const { connect } = require('net'); + +// This test validates that the server returns 408 +// after server.headersTimeout if the client +// pauses before start sending the request. + +let sendDelayedRequestHeaders; +const headersTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout, + requestTimeout: 0, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustNotCall()); +server.on('connection', common.mustCall(() => { + assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); + sendDelayedRequestHeaders(); +})); + +assert.strictEqual(server.headersTimeout, headersTimeout); + +// Check that timeout event is not triggered +server.once('timeout', common.mustNotCall((socket) => { + socket.destroy(); +})); + +server.listen(0, common.mustCall(() => { + const client = connect(server.address().port); + let response = ''; + + client.on('data', common.mustCall((chunk) => { + response += chunk.toString('utf-8'); + })); + + const errOrEnd = common.mustSucceed(function(err) { + assert.strictEqual( + response, + 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' + ); + server.close(); + }); + + client.on('end', errOrEnd); + client.on('error', errOrEnd); + + client.resume(); + + sendDelayedRequestHeaders = common.mustCall(() => { + setTimeout(() => { + client.write('POST / HTTP/1.1\r\n'); + client.write('Content-Length: 20\r\n'); + client.write('Connection: close\r\n\r\n'); + client.write('12345678901234567890\r\n\r\n'); + }, common.platformTimeout(headersTimeout * 2)).unref(); + }); +})); diff --git a/test/parallel/test-http-server-headers-timeout-interrupted-headers.js b/test/parallel/test-http-server-headers-timeout-interrupted-headers.js new file mode 100644 index 00000000000..ea47ae8e19e --- /dev/null +++ b/test/parallel/test-http-server-headers-timeout-interrupted-headers.js @@ -0,0 +1,61 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { createServer } = require('http'); +const { connect } = require('net'); + +// This test validates that the server returns 408 +// after server.headersTimeout if the client +// pauses sending in the middle of a header. + +let sendDelayedRequestHeaders; +const headersTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout, + requestTimeout: 0, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustNotCall()); +server.on('connection', common.mustCall(() => { + assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); + sendDelayedRequestHeaders(); +})); + +assert.strictEqual(server.headersTimeout, headersTimeout); + +// Check that timeout event is not triggered +server.once('timeout', common.mustNotCall((socket) => { + socket.destroy(); +})); + +server.listen(0, common.mustCall(() => { + const client = connect(server.address().port); + let response = ''; + + client.on('data', common.mustCall((chunk) => { + response += chunk.toString('utf-8'); + })); + + const errOrEnd = common.mustSucceed(function(err) { + assert.strictEqual( + response, + 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' + ); + server.close(); + }); + + client.on('end', errOrEnd); + client.on('error', errOrEnd); + + client.resume(); + client.write('GET / HTTP/1.1\r\n'); + client.write('Connection: close\r\n'); + client.write('X-CRASH: '); + + sendDelayedRequestHeaders = common.mustCall(() => { + setTimeout(() => { + client.write('1234567890\r\n\r\n'); + }, common.platformTimeout(headersTimeout * 2)).unref(); + }); +})); diff --git a/test/parallel/test-http-server-headers-timeout-keepalive.js b/test/parallel/test-http-server-headers-timeout-keepalive.js new file mode 100644 index 00000000000..c7e5e56da90 --- /dev/null +++ b/test/parallel/test-http-server-headers-timeout-keepalive.js @@ -0,0 +1,103 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { createServer } = require('http'); +const { connect } = require('net'); + +// This test validates that the server returns 408 +// after server.headersTimeout if the client +// does not send headers before the timeout, and +// that keep alive works properly. + +function performRequestWithDelay(client, firstDelay, secondDelay, closeAfter) { + client.resume(); + client.write('GET / HTTP/1.1\r\n'); + + firstDelay = common.platformTimeout(firstDelay); + secondDelay = common.platformTimeout(secondDelay); + + setTimeout(() => { + client.write('Connection: '); + }, firstDelay).unref(); + + // Complete the request + setTimeout(() => { + client.write(`${closeAfter ? 'close' : 'keep-alive'}\r\n\r\n`); + }, firstDelay + secondDelay).unref(); +} + +const headersTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout, + requestTimeout: 0, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustCallAtLeast((req, res) => { + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end(); +})); + +assert.strictEqual(server.headersTimeout, headersTimeout); + +// Check that timeout event is not triggered +server.once('timeout', common.mustNotCall((socket) => { + socket.destroy(); +})); + +server.listen(0, common.mustCall(() => { + const client = connect(server.address().port); + let second = false; + let response = ''; + + client.on('data', common.mustCallAtLeast((chunk) => { + response += chunk.toString('utf-8'); + + // First response has ended + if (!second && response.endsWith('\r\n\r\n')) { + assert.strictEqual( + response.split('\r\n')[0], + 'HTTP/1.1 200 OK' + ); + + const defer = common.platformTimeout(headersTimeout * 1.5); + + // Wait some time to make sure headersTimeout + // does not interfere with keep alive + setTimeout(() => { + response = ''; + second = true; + + // Perform a second request expected to finish after headersTimeout + performRequestWithDelay( + client, + headersTimeout / 5, + headersTimeout, + true + ); + }, defer).unref(); + } + }, 1)); + + const errOrEnd = common.mustCall(function(err) { + assert.strictEqual(second, true); + assert.strictEqual( + response, + // Empty because of https://github.com/nodejs/node/commit/e8d7fedf7cad6e612e4f2e0456e359af57608ac7 + // 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' + '' + ); + server.close(); + }); + + client.on('error', errOrEnd); + client.on('end', errOrEnd); + + // Perform a second request expected to finish before headersTimeout + performRequestWithDelay( + client, + headersTimeout / 5, + headersTimeout / 5, + false + ); +})); diff --git a/test/parallel/test-http-server-headers-timeout-pipelining.js b/test/parallel/test-http-server-headers-timeout-pipelining.js new file mode 100644 index 00000000000..13f89c60c05 --- /dev/null +++ b/test/parallel/test-http-server-headers-timeout-pipelining.js @@ -0,0 +1,76 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { createServer } = require('http'); +const { connect } = require('net'); + +// This test validates that the server returns 408 +// after server.requestTimeout if the client +// does not complete a request when using pipelining. + +const headersTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout, + requestTimeout: 0, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustCallAtLeast((req, res) => { + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end(); +})); + +// 0 seconds is the default +assert.strictEqual(server.headersTimeout, headersTimeout); + +// Make sure requestTimeout and keepAliveTimeout +// are big enough for the headersTimeout. +server.requestTimeout = 0; +server.keepAliveTimeout = 0; + +server.listen(0, common.mustCall(() => { + const client = connect(server.address().port); + let second = false; + let response = ''; + + client.on('data', common.mustCallAtLeast((chunk) => { + response += chunk.toString('utf-8'); + + // First response has ended + if (!second && response.endsWith('\r\n\r\n')) { + assert.strictEqual( + response.split('\r\n')[0], + 'HTTP/1.1 200 OK' + ); + + response = ''; + second = true; + } + }, 1)); + + const errOrEnd = common.mustCall(function(err) { + if (!second) { + return; + } + + assert.strictEqual( + response, + // Empty because of https://github.com/nodejs/node/commit/e8d7fedf7cad6e612e4f2e0456e359af57608ac7 + // 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' + '' + ); + server.close(); + }); + + client.on('error', errOrEnd); + client.on('end', errOrEnd); + + // Send two requests using pipelining. Delay before finishing the second one + client.resume(); + client.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\nGET / HTTP/1.1\r\nConnection: '); + + // Complete the request + setTimeout(() => { + client.write('close\r\n\r\n'); + }, headersTimeout * 1.5).unref(); +})); diff --git a/test/parallel/test-http-server-request-timeout-delayed-body.js b/test/parallel/test-http-server-request-timeout-delayed-body.js index ec8ebbb5cd1..c5ecb720e47 100644 --- a/test/parallel/test-http-server-request-timeout-delayed-body.js +++ b/test/parallel/test-http-server-request-timeout-delayed-body.js @@ -10,7 +10,13 @@ const { connect } = require('net'); // pauses before start sending the body. let sendDelayedRequestBody; -const server = createServer(common.mustCall((req, res) => { +const requestTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout: 0, + requestTimeout, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustCall((req, res) => { let body = ''; req.setEncoding('utf-8'); @@ -28,10 +34,6 @@ const server = createServer(common.mustCall((req, res) => { sendDelayedRequestBody(); })); -// 0 seconds is the default -assert.strictEqual(server.requestTimeout, 0); -const requestTimeout = common.platformTimeout(1000); -server.requestTimeout = requestTimeout; assert.strictEqual(server.requestTimeout, requestTimeout); server.listen(0, common.mustCall(() => { @@ -51,11 +53,10 @@ server.listen(0, common.mustCall(() => { sendDelayedRequestBody = common.mustCall(() => { setTimeout(() => { client.write('12345678901234567890\r\n\r\n'); - }, common.platformTimeout(2000)).unref(); + }, common.platformTimeout(requestTimeout * 2)).unref(); }); - const errOrEnd = common.mustCall(function(err) { - console.log(err); + const errOrEnd = common.mustSucceed(function(err) { assert.strictEqual( response, 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' diff --git a/test/parallel/test-http-server-request-timeout-delayed-headers.js b/test/parallel/test-http-server-request-timeout-delayed-headers.js index f5d85b70514..7174afec47f 100644 --- a/test/parallel/test-http-server-request-timeout-delayed-headers.js +++ b/test/parallel/test-http-server-request-timeout-delayed-headers.js @@ -8,16 +8,20 @@ const { connect } = require('net'); // This test validates that the server returns 408 // after server.requestTimeout if the client // pauses before start sending the request. + let sendDelayedRequestHeaders; -const server = createServer(common.mustNotCall()); +const requestTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout: 0, + requestTimeout, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustNotCall()); server.on('connection', common.mustCall(() => { assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); sendDelayedRequestHeaders(); })); -// 0 seconds is the default -assert.strictEqual(server.requestTimeout, 0); -const requestTimeout = common.platformTimeout(1000); -server.requestTimeout = requestTimeout; + assert.strictEqual(server.requestTimeout, requestTimeout); server.listen(0, common.mustCall(() => { @@ -28,8 +32,7 @@ server.listen(0, common.mustCall(() => { response += chunk.toString('utf-8'); })); - const errOrEnd = common.mustCall(function(err) { - console.log(err); + const errOrEnd = common.mustSucceed(function(err) { assert.strictEqual( response, 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' @@ -48,6 +51,6 @@ server.listen(0, common.mustCall(() => { client.write('Content-Length: 20\r\n'); client.write('Connection: close\r\n\r\n'); client.write('12345678901234567890\r\n\r\n'); - }, common.platformTimeout(2000)).unref(); + }, common.platformTimeout(requestTimeout * 2)).unref(); }); })); diff --git a/test/parallel/test-http-server-request-timeout-interrupted-body.js b/test/parallel/test-http-server-request-timeout-interrupted-body.js index 7d70351fdc0..ee087719e4e 100644 --- a/test/parallel/test-http-server-request-timeout-interrupted-body.js +++ b/test/parallel/test-http-server-request-timeout-interrupted-body.js @@ -8,8 +8,15 @@ const { connect } = require('net'); // This test validates that the server returns 408 // after server.requestTimeout if the client // pauses sending in the middle of the body. + let sendDelayedRequestBody; -const server = createServer(common.mustCall((req, res) => { +const requestTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout: 0, + requestTimeout, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustCall((req, res) => { let body = ''; req.setEncoding('utf-8'); @@ -27,10 +34,6 @@ const server = createServer(common.mustCall((req, res) => { sendDelayedRequestBody(); })); -// 0 seconds is the default -assert.strictEqual(server.requestTimeout, 0); -const requestTimeout = common.platformTimeout(1000); -server.requestTimeout = requestTimeout; assert.strictEqual(server.requestTimeout, requestTimeout); server.listen(0, common.mustCall(() => { @@ -41,8 +44,7 @@ server.listen(0, common.mustCall(() => { response += chunk.toString('utf-8'); })); - const errOrEnd = common.mustCall(function(err) { - console.log(err); + const errOrEnd = common.mustSucceed(function(err) { assert.strictEqual( response, 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' @@ -63,6 +65,6 @@ server.listen(0, common.mustCall(() => { sendDelayedRequestBody = common.mustCall(() => { setTimeout(() => { client.write('1234567890\r\n\r\n'); - }, common.platformTimeout(2000)).unref(); + }, common.platformTimeout(requestTimeout * 2)).unref(); }); })); diff --git a/test/parallel/test-http-server-request-timeout-interrupted-headers.js b/test/parallel/test-http-server-request-timeout-interrupted-headers.js index 1fa8946ffae..64511c6b50c 100644 --- a/test/parallel/test-http-server-request-timeout-interrupted-headers.js +++ b/test/parallel/test-http-server-request-timeout-interrupted-headers.js @@ -8,17 +8,20 @@ const { connect } = require('net'); // This test validates that the server returns 408 // after server.requestTimeout if the client // pauses sending in the middle of a header. + let sendDelayedRequestHeaders; -const server = createServer(common.mustNotCall()); +const requestTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout: 0, + requestTimeout, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustNotCall()); server.on('connection', common.mustCall(() => { assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); sendDelayedRequestHeaders(); })); -// 120 seconds is the default -assert.strictEqual(server.requestTimeout, 0); -const requestTimeout = common.platformTimeout(1000); -server.requestTimeout = requestTimeout; assert.strictEqual(server.requestTimeout, requestTimeout); server.listen(0, common.mustCall(() => { @@ -29,8 +32,7 @@ server.listen(0, common.mustCall(() => { response += chunk.toString('utf-8'); })); - const errOrEnd = common.mustCall(function(err) { - console.log(err); + const errOrEnd = common.mustSucceed(function(err) { assert.strictEqual( response, 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' @@ -49,6 +51,6 @@ server.listen(0, common.mustCall(() => { sendDelayedRequestHeaders = common.mustCall(() => { setTimeout(() => { client.write('1234567890\r\n\r\n'); - }, common.platformTimeout(2000)).unref(); + }, common.platformTimeout(requestTimeout * 2)).unref(); }); })); diff --git a/test/parallel/test-http-server-request-timeout-keepalive.js b/test/parallel/test-http-server-request-timeout-keepalive.js index 77fde867e9b..0b8f798c3eb 100644 --- a/test/parallel/test-http-server-request-timeout-keepalive.js +++ b/test/parallel/test-http-server-request-timeout-keepalive.js @@ -8,36 +8,36 @@ const { connect } = require('net'); // This test validates that the server returns 408 // after server.requestTimeout if the client // does not complete a request, and that keep alive -// works properly +// works properly. -function performRequestWithDelay(client, firstDelay, secondDelay) { +function performRequestWithDelay(client, firstDelay, secondDelay, closeAfter) { client.resume(); client.write('GET / HTTP/1.1\r\n'); firstDelay = common.platformTimeout(firstDelay); secondDelay = common.platformTimeout(secondDelay); - console.log('performRequestWithDelay', firstDelay, secondDelay); - setTimeout(() => { client.write('Connection: '); }, firstDelay).unref(); // Complete the request setTimeout(() => { - client.write('keep-alive\r\n\r\n'); + client.write(`${closeAfter ? 'close' : 'keep-alive'}\r\n\r\n`); }, firstDelay + secondDelay).unref(); } -const server = createServer(common.mustCallAtLeast((req, res) => { +const requestTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout: 0, + requestTimeout, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustCallAtLeast((req, res) => { res.writeHead(200, { 'Content-Type': 'text/plain' }); res.end(); })); -// 0 seconds is the default -assert.strictEqual(server.requestTimeout, 0); -const requestTimeout = common.platformTimeout(1000); -server.requestTimeout = requestTimeout; assert.strictEqual(server.requestTimeout, requestTimeout); // Make sure keepAliveTimeout is big enough for the requestTimeout. @@ -58,9 +58,7 @@ server.listen(0, common.mustCall(() => { 'HTTP/1.1 200 OK' ); - const defer = common.platformTimeout(server.requestTimeout * 1.5); - - console.log('defer by', defer); + const defer = common.platformTimeout(requestTimeout * 1.5); // Wait some time to make sure requestTimeout // does not interfere with keep alive @@ -69,13 +67,17 @@ server.listen(0, common.mustCall(() => { second = true; // Perform a second request expected to finish after requestTimeout - performRequestWithDelay(client, 1000, 3000); + performRequestWithDelay( + client, + requestTimeout / 5, + requestTimeout, + true + ); }, defer).unref(); } }, 1)); const errOrEnd = common.mustCall(function(err) { - console.log(err); assert.strictEqual(second, true); assert.strictEqual( response, @@ -90,5 +92,10 @@ server.listen(0, common.mustCall(() => { client.on('end', errOrEnd); // Perform a second request expected to finish before requestTimeout - performRequestWithDelay(client, 50, 500); + performRequestWithDelay( + client, + requestTimeout / 5, + requestTimeout / 5, + false + ); })); diff --git a/test/parallel/test-http-server-request-timeout-pipelining.js b/test/parallel/test-http-server-request-timeout-pipelining.js new file mode 100644 index 00000000000..4e6977b3270 --- /dev/null +++ b/test/parallel/test-http-server-request-timeout-pipelining.js @@ -0,0 +1,70 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { createServer } = require('http'); +const { connect } = require('net'); + +// This test validates that the server returns 408 +// after server.requestTimeout if the client +// does not complete a request when using pipelining. + +const requestTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout: 0, + requestTimeout, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustCallAtLeast((req, res) => { + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end(); +})); + +assert.strictEqual(server.requestTimeout, requestTimeout); + +server.listen(0, common.mustCall(() => { + const client = connect(server.address().port); + let second = false; + let response = ''; + + client.on('data', common.mustCallAtLeast((chunk) => { + response += chunk.toString('utf-8'); + + // First response has ended + if (!second && response.endsWith('\r\n\r\n')) { + assert.strictEqual( + response.split('\r\n')[0], + 'HTTP/1.1 200 OK' + ); + + response = ''; + second = true; + } + }, 1)); + + const errOrEnd = common.mustCall(function(err) { + if (!second) { + return; + } + + assert.strictEqual( + response, + // Empty because of https://github.com/nodejs/node/commit/e8d7fedf7cad6e612e4f2e0456e359af57608ac7 + // 'HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n' + '' + ); + server.close(); + }); + + client.on('error', errOrEnd); + client.on('end', errOrEnd); + + // Send two requests using pipelining. Delay before finishing the second one + client.resume(); + client.write('GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\nGET / HTTP/1.1\r\nConnection: '); + + // Complete the request + setTimeout(() => { + client.write('close\r\n\r\n'); + }, requestTimeout * 1.5).unref(); +})); diff --git a/test/parallel/test-http-server-request-timeout-upgrade.js b/test/parallel/test-http-server-request-timeout-upgrade.js index 87e8dbab131..b1974a128b9 100644 --- a/test/parallel/test-http-server-request-timeout-upgrade.js +++ b/test/parallel/test-http-server-request-timeout-upgrade.js @@ -8,16 +8,18 @@ const { connect } = require('net'); // This test validates that the requestTimeoout // is disabled after the connection is upgraded. let sendDelayedRequestHeaders; -const server = createServer(common.mustNotCall()); +const requestTimeout = common.platformTimeout(1000); +const server = createServer({ + headersTimeout: 0, + requestTimeout, + keepAliveTimeout: 0, + connectionsCheckingInterval: common.platformTimeout(250), +}, common.mustNotCall()); server.on('connection', common.mustCall(() => { assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); sendDelayedRequestHeaders(); })); -// 0 seconds is the default -assert.strictEqual(server.requestTimeout, 0); -const requestTimeout = common.platformTimeout(1000); -server.requestTimeout = requestTimeout; assert.strictEqual(server.requestTimeout, requestTimeout); server.on('upgrade', common.mustCall((req, socket, head) => { diff --git a/test/parallel/test-http-slow-headers-keepalive-multiple-requests.js b/test/parallel/test-http-slow-headers-keepalive-multiple-requests.js deleted file mode 100644 index 9ea76e8e56e..00000000000 --- a/test/parallel/test-http-slow-headers-keepalive-multiple-requests.js +++ /dev/null @@ -1,51 +0,0 @@ -'use strict'; - -const common = require('../common'); -const http = require('http'); -const net = require('net'); -const { finished } = require('stream'); - -const headers = - 'GET / HTTP/1.1\r\n' + - 'Host: localhost\r\n' + - 'Connection: keep-alive\r\n' + - 'Agent: node\r\n'; - -const baseTimeout = 1000; - -const server = http.createServer(common.mustCall((req, res) => { - req.resume(); - res.writeHead(200); - res.end(); -}, 2)); - -server.keepAliveTimeout = 10 * baseTimeout; -server.headersTimeout = baseTimeout; - -server.once('timeout', common.mustNotCall((socket) => { - socket.destroy(); -})); - -server.listen(0, () => { - const client = net.connect(server.address().port); - - // first request - client.write(headers); - client.write('\r\n'); - - setTimeout(() => { - // second request - client.write(headers); - // `headersTimeout` doesn't seem to fire if request - // is sent altogether. - setTimeout(() => { - client.write('\r\n'); - client.end(); - }, 10); - }, baseTimeout + 10); - - client.resume(); - finished(client, common.mustCall((err) => { - server.close(); - })); -}); diff --git a/test/parallel/test-http-slow-headers-keepalive.js b/test/parallel/test-http-slow-headers-keepalive.js deleted file mode 100644 index 4958d534ef7..00000000000 --- a/test/parallel/test-http-slow-headers-keepalive.js +++ /dev/null @@ -1,51 +0,0 @@ -'use strict'; - -const common = require('../common'); -const http = require('http'); -const net = require('net'); -const { finished } = require('stream'); - -const headers = - 'GET / HTTP/1.1\r\n' + - 'Host: localhost\r\n' + - 'Connection: keep-alive\r\n' + - 'Agent: node\r\n'; - -let sendCharEvery = 1000; - -const server = http.createServer(common.mustCall((req, res) => { - res.writeHead(200); - res.end(); -})); - -// Pass a REAL env variable to shortening up the default -// value which is 40s otherwise this is useful for manual -// testing -if (!process.env.REAL) { - sendCharEvery = common.platformTimeout(10); - server.headersTimeout = 2 * sendCharEvery; -} - -server.once('timeout', common.mustCall((socket) => { - socket.destroy(); -})); - -server.listen(0, () => { - const client = net.connect(server.address().port); - client.write(headers); - // Finish the first request - client.write('\r\n'); - // second request - client.write(headers); - client.write('X-CRASH: '); - - const interval = setInterval(() => { - client.write('a'); - }, sendCharEvery); - - client.resume(); - finished(client, common.mustCall((err) => { - clearInterval(interval); - server.close(); - })); -}); diff --git a/test/parallel/test-http-slow-headers.js b/test/parallel/test-http-slow-headers.js deleted file mode 100644 index 25ee5b63e8c..00000000000 --- a/test/parallel/test-http-slow-headers.js +++ /dev/null @@ -1,50 +0,0 @@ -'use strict'; - -const common = require('../common'); -const assert = require('assert'); -const { createServer } = require('http'); -const { connect } = require('net'); -const { finished } = require('stream'); - -// This test validates that the 'timeout' event fires -// after server.headersTimeout. - -const headers = - 'GET / HTTP/1.1\r\n' + - 'Host: localhost\r\n' + - 'Agent: node\r\n'; - -const server = createServer(common.mustNotCall()); -let sendCharEvery = 1000; - -// 60 seconds is the default -assert.strictEqual(server.headersTimeout, 60 * 1000); - -// Pass a REAL env variable to shortening up the default -// value which is 40s otherwise this is useful for manual -// testing -if (!process.env.REAL) { - sendCharEvery = common.platformTimeout(10); - server.headersTimeout = 2 * sendCharEvery; -} - -server.once('timeout', common.mustCall((socket) => { - socket.destroy(); -})); - -server.listen(0, common.mustCall(() => { - const client = connect(server.address().port); - client.write(headers); - client.write('X-CRASH: '); - - const interval = setInterval(() => { - client.write('a'); - }, sendCharEvery); - - client.resume(); - - finished(client, common.mustCall((err) => { - clearInterval(interval); - server.close(); - })); -})); diff --git a/test/parallel/test-https-server-headers-timeout.js b/test/parallel/test-https-server-headers-timeout.js new file mode 100644 index 00000000000..45457e39425 --- /dev/null +++ b/test/parallel/test-https-server-headers-timeout.js @@ -0,0 +1,21 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const { createServer } = require('https'); +const fixtures = require('../common/fixtures'); + +const options = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), +}; + +const server = createServer(options); + +// 60000 seconds is the default +assert.strictEqual(server.headersTimeout, 60000); +const headersTimeout = common.platformTimeout(1000); +server.headersTimeout = headersTimeout; +assert.strictEqual(server.headersTimeout, headersTimeout); diff --git a/test/parallel/test-https-server-request-timeout.js b/test/parallel/test-https-server-request-timeout.js index 66a1cb9f25f..00bac8ea399 100644 --- a/test/parallel/test-https-server-request-timeout.js +++ b/test/parallel/test-https-server-request-timeout.js @@ -14,8 +14,8 @@ const options = { const server = createServer(options); -// 0 seconds is the default -assert.strictEqual(server.requestTimeout, 0); +// 300 seconds is the default +assert.strictEqual(server.requestTimeout, 300000); const requestTimeout = common.platformTimeout(1000); server.requestTimeout = requestTimeout; assert.strictEqual(server.requestTimeout, requestTimeout); diff --git a/test/parallel/test-https-slow-headers.js b/test/parallel/test-https-slow-headers.js deleted file mode 100644 index 95f0caa9878..00000000000 --- a/test/parallel/test-https-slow-headers.js +++ /dev/null @@ -1,63 +0,0 @@ -'use strict'; - -const common = require('../common'); -const { readKey } = require('../common/fixtures'); - -if (!common.hasCrypto) - common.skip('missing crypto'); - -const assert = require('assert'); -const { createServer } = require('https'); -const { connect } = require('tls'); -const { finished } = require('stream'); - -// This test validates that the 'timeout' event fires -// after server.headersTimeout. - -const headers = - 'GET / HTTP/1.1\r\n' + - 'Host: localhost\r\n' + - 'Agent: node\r\n'; - -const server = createServer({ - key: readKey('agent1-key.pem'), - cert: readKey('agent1-cert.pem'), - ca: readKey('ca1-cert.pem'), -}, common.mustNotCall()); - -let sendCharEvery = 1000; - -// 60 seconds is the default -assert.strictEqual(server.headersTimeout, 60 * 1000); - -// Pass a REAL env variable to shortening up the default -// value which is 40s otherwise -// this is useful for manual testing -if (!process.env.REAL) { - sendCharEvery = common.platformTimeout(10); - server.headersTimeout = 2 * sendCharEvery; -} - -server.once('timeout', common.mustCall((socket) => { - socket.destroy(); -})); - -server.listen(0, common.mustCall(() => { - const client = connect({ - port: server.address().port, - rejectUnauthorized: false - }); - client.write(headers); - client.write('X-CRASH: '); - - const interval = setInterval(() => { - client.write('a'); - }, sendCharEvery); - - client.resume(); - - finished(client, common.mustCall((err) => { - clearInterval(interval); - server.close(); - })); -})); |