From e38a0c7705772e441d2a9c581a41e52bb018212b Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 18 Apr 2018 09:26:16 +0200 Subject: [PATCH 1/4] http: cleanup _http_common.js Remove constant deep property access by storing a reference, use more const, make some conditionals stricter. --- lib/_http_common.js | 51 +++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/lib/_http_common.js b/lib/_http_common.js index 7f53aca246b040..f34396ba8a8a62 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -65,12 +65,12 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, const parser = this; const { socket } = parser; - if (!headers) { + if (headers === undefined) { headers = parser._headers; parser._headers = []; } - if (!url) { + if (url === undefined) { url = parser._url; parser._url = ''; } @@ -80,12 +80,12 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, socket.server[kIncomingMessage]) || IncomingMessage; - parser.incoming = new ParserIncomingMessage(socket); - parser.incoming.httpVersionMajor = versionMajor; - parser.incoming.httpVersionMinor = versionMinor; - parser.incoming.httpVersion = `${versionMajor}.${versionMinor}`; - parser.incoming.url = url; - parser.incoming.upgrade = upgrade; + const incoming = parser.incoming = new ParserIncomingMessage(socket); + incoming.httpVersionMajor = versionMajor; + incoming.httpVersionMinor = versionMinor; + incoming.httpVersion = `${versionMajor}.${versionMinor}`; + incoming.url = url; + incoming.upgrade = upgrade; var n = headers.length; @@ -93,51 +93,48 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, if (parser.maxHeaderPairs > 0) n = Math.min(n, parser.maxHeaderPairs); - parser.incoming._addHeaderLines(headers, n); + incoming._addHeaderLines(headers, n); if (typeof method === 'number') { // server only - parser.incoming.method = methods[method]; + incoming.method = methods[method]; } else { // client only - parser.incoming.statusCode = statusCode; - parser.incoming.statusMessage = statusMessage; + incoming.statusCode = statusCode; + incoming.statusMessage = statusMessage; } - return parser.onIncoming(parser.incoming, shouldKeepAlive); + return parser.onIncoming(incoming, shouldKeepAlive); } // XXX This is a mess. // TODO: http.Parser should be a Writable emits request/response events. function parserOnBody(b, start, len) { - var parser = this; - var stream = parser.incoming; + const stream = this.incoming; // if the stream has already been removed, then drop it. - if (!stream) + if (stream === null) return; - var socket = stream.socket; - // pretend this was the result of a stream._read call. if (len > 0 && !stream._dumped) { var slice = b.slice(start, start + len); var ret = stream.push(slice); if (!ret) - readStop(socket); + readStop(this.socket); } } function parserOnMessageComplete() { - var parser = this; - var stream = parser.incoming; + const parser = this; + const stream = parser.incoming; - if (stream) { + if (stream !== null) { stream.complete = true; // Emit any trailing headers. - var headers = parser._headers; - if (headers) { - parser.incoming._addHeaderLines(headers, headers.length); + const headers = parser._headers; + if (headers.length) { + stream._addHeaderLines(headers, headers.length); parser._headers = []; parser._url = ''; } @@ -151,8 +148,8 @@ function parserOnMessageComplete() { } -var parsers = new FreeList('parsers', 1000, function() { - var parser = new HTTPParser(HTTPParser.REQUEST); +const parsers = new FreeList('parsers', 1000, function() { + const parser = new HTTPParser(HTTPParser.REQUEST); parser._headers = []; parser._url = ''; From b97117ee4f1d7ee1702e54ac9a5ccbc4c5449f8a Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 18 Apr 2018 09:28:16 +0200 Subject: [PATCH 2/4] http: remove duplicate comment A comment explaining kOnHeaders function of the parser was duplicated in two places but the second instance was more confusing than helpful. --- lib/_http_common.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/_http_common.js b/lib/_http_common.js index f34396ba8a8a62..c8998f4d85a3e8 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -159,11 +159,6 @@ const parsers = new FreeList('parsers', 1000, function() { parser.incoming = null; parser.outgoing = null; - // Only called in the slow case where slow means - // that the request headers were either fragmented - // across multiple TCP packets or too large to be - // processed in a single run. This method is also - // called to process trailing HTTP headers. parser[kOnHeaders] = parserOnHeaders; parser[kOnHeadersComplete] = parserOnHeadersComplete; parser[kOnBody] = parserOnBody; From dbe4a97fff7f1149d60149b999f2c722332a87db Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 18 Apr 2018 10:17:06 +0200 Subject: [PATCH 3/4] http: cleanup parser properties Cleanup constructor and freeParser to manage all existing parser properties, not just some. --- lib/_http_client.js | 4 ---- lib/_http_common.js | 7 +++++++ lib/_http_server.js | 4 ---- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index c3ef9de20498df..22ae85c92782ec 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -606,7 +606,6 @@ function tickOnSocket(req, socket) { req.connection = socket; parser.reinitialize(HTTPParser.RESPONSE); parser.socket = socket; - parser.incoming = null; parser.outgoing = req; req.parser = parser; @@ -619,9 +618,6 @@ function tickOnSocket(req, socket) { // Propagate headers limit from request object to parser if (typeof req.maxHeadersCount === 'number') { parser.maxHeaderPairs = req.maxHeadersCount << 1; - } else { - // Set default value because parser may be reused from FreeList - parser.maxHeaderPairs = 2000; } parser.onIncoming = parserOnIncomingClient; diff --git a/lib/_http_common.js b/lib/_http_common.js index c8998f4d85a3e8..7eb37511bd12c7 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -41,6 +41,8 @@ const kOnBody = HTTPParser.kOnBody | 0; const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0; const kOnExecute = HTTPParser.kOnExecute | 0; +const MAX_HEADER_PAIRS = 2000; + // Only called in the slow case where slow means // that the request headers were either fragmented // across multiple TCP packets or too large to be @@ -159,6 +161,9 @@ const parsers = new FreeList('parsers', 1000, function() { parser.incoming = null; parser.outgoing = null; + parser.maxHeaderPairs = MAX_HEADER_PAIRS; + + parser.onIncoming = null; parser[kOnHeaders] = parserOnHeaders; parser[kOnHeadersComplete] = parserOnHeadersComplete; parser[kOnBody] = parserOnBody; @@ -180,6 +185,8 @@ function closeParserInstance(parser) { parser.close(); } function freeParser(parser, req, socket) { if (parser) { parser._headers = []; + parser._url = ''; + parser.maxHeaderPairs = MAX_HEADER_PAIRS; parser.onIncoming = null; if (parser._consumed) parser.unconsume(); diff --git a/lib/_http_server.js b/lib/_http_server.js index a8389dbda62f0d..af3d01285567aa 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -342,14 +342,10 @@ function connectionListenerInternal(server, socket) { parser.reinitialize(HTTPParser.REQUEST); parser.socket = socket; socket.parser = parser; - parser.incoming = null; // Propagate headers limit from server instance to parser if (typeof server.maxHeadersCount === 'number') { parser.maxHeaderPairs = server.maxHeadersCount << 1; - } else { - // Set default value because parser may be reused from FreeList - parser.maxHeaderPairs = 2000; } var state = { From 76f71e081f283e56deb58cb58ec47a0d75fdeec7 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 18 Apr 2018 10:20:13 +0200 Subject: [PATCH 4/4] http: remove duplicate parser unset freeParser already unsets parser property of socket if socket is passed in specifically. There's no need to do this twice. --- lib/_http_common.js | 2 -- lib/_http_server.js | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/_http_common.js b/lib/_http_common.js index 7eb37511bd12c7..ab075dc817ed6f 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -191,8 +191,6 @@ function freeParser(parser, req, socket) { if (parser._consumed) parser.unconsume(); parser._consumed = false; - if (parser.socket) - parser.socket.parser = null; parser.socket = null; parser.incoming = null; parser.outgoing = null; diff --git a/lib/_http_server.js b/lib/_http_server.js index af3d01285567aa..9087e6bb8a2168 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -522,7 +522,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) { socket.removeListener('error', socketOnError); unconsume(parser, socket); parser.finish(); - freeParser(parser, req, null); + freeParser(parser, req, socket); parser = null; var eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade';