From f8f8754d43ece907a400edf673904f222048c2d6 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 22 Aug 2019 08:39:07 +0200 Subject: [PATCH 1/8] Revert "http: reset parser.incoming when server response is finished" This reverts commit 779a05d5d1bfe2eeb05386f6415d36f80ca0b3b5. PR-URL: https://github.com/nodejs/node/pull/29263 Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell Reviewed-By: Luigi Pinca --- lib/_http_server.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 68492f3eaaeec0..f7b7752b21ae49 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -621,8 +621,6 @@ function resOnFinish(req, res, socket, state, server) { assert(state.incoming.length === 0 || state.incoming[0] === req); state.incoming.shift(); - // Reset the .incoming property so that the request object can be gc'ed. - if (socket.parser) socket.parser.incoming = null; // If the user never called req.read(), and didn't pipe() or // .resume() or .on('data'), then we call req._dump() so that the From 3c7a1a9090b4ae0353d0d2cafbdfed91f041904e Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 22 Aug 2019 11:42:46 +0200 Subject: [PATCH 2/8] test, http: add regression test for keepalive 'end' event This test covers a regression where 'end' was not emitted in the case of keepalive requests without parsing the full body. PR-URL: https://github.com/nodejs/node/pull/29263 Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell Reviewed-By: Luigi Pinca --- .../test-http-server-keepalive-end.js | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 test/parallel/test-http-server-keepalive-end.js diff --git a/test/parallel/test-http-server-keepalive-end.js b/test/parallel/test-http-server-keepalive-end.js new file mode 100644 index 00000000000000..1e3a44d368a020 --- /dev/null +++ b/test/parallel/test-http-server-keepalive-end.js @@ -0,0 +1,29 @@ +'use strict'; + +const common = require('../common'); +const { createServer } = require('http'); +const { connect } = require('net'); + +const server = createServer(common.mustCall((req, res) => { + req.on('end', common.mustCall()); + res.end('hello world'); +})); + +server.unref(); + +server.listen(0, common.mustCall(() => { + + const client = connect(server.address().port); + + const req = [ + 'POST / HTTP/1.1', + `Host: localhost:${server.address().port}`, + 'Connection: keep-alive', + 'Content-Length: 11', + '', + 'hello world', + '' + ].join('\r\n'); + + client.end(req); +})); From ff6330a6ac305fda67cab6ddfa87bb2e7e7785c7 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 20 Aug 2019 17:24:39 -0400 Subject: [PATCH 3/8] test: fix 'timeout' typos I don't think so, Tim. PR-URL: https://github.com/nodejs/node/pull/29234 Reviewed-By: Richard Lau Reviewed-By: Rich Trott Reviewed-By: Yongsheng Zhang Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- test/common/index.js | 2 +- test/common/tmpdir.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 13604d06e14a36..98a26872223cb9 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -401,7 +401,7 @@ function canCreateSymLink() { 'System32', 'whoami.exe'); try { - const output = execSync(`${whoamiPath} /priv`, { timout: 1000 }); + const output = execSync(`${whoamiPath} /priv`, { timeout: 1000 }); return output.includes('SeCreateSymbolicLinkPrivilege'); } catch { return false; diff --git a/test/common/tmpdir.js b/test/common/tmpdir.js index 33b2264a8d69f5..2be6e530275fbd 100644 --- a/test/common/tmpdir.js +++ b/test/common/tmpdir.js @@ -28,7 +28,7 @@ function rimrafSync(pathname, { spawn = true } = {}) { if (spawn && process.platform === 'win32' && st.isDirectory()) { try { // Try `rmdir` first. - execSync(`rmdir /q /s ${pathname}`, { timout: 1000 }); + execSync(`rmdir /q /s ${pathname}`, { timeout: 1000 }); } catch (e) { // Attempt failed. Log and carry on. debug(e); From 3cc8fca2990d8d09103a8a243be1092ac578d001 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 20 Aug 2019 14:10:42 +0200 Subject: [PATCH 4/8] crypto: handle i2d_SSL_SESSION() error return i2d_SSL_SESSION() can return a value <= 0 when the session is malformed or otherwise invalid. Handle that case. This change comes without a regression test because I couldn't figure out a good way to generate an existing but invalid session in a timely fashion. Fixes: https://github.com/nodejs/node/issues/29202 PR-URL: https://github.com/nodejs/node/pull/29225 Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- src/node_crypto.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 70da2e310ea15c..78d89b71f4c317 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2317,11 +2317,12 @@ void SSLWrap::GetSession(const FunctionCallbackInfo& args) { return; int slen = i2d_SSL_SESSION(sess, nullptr); - CHECK_GT(slen, 0); + if (slen <= 0) + return; // Invalid or malformed session. AllocatedBuffer sbuf = env->AllocateManaged(slen); unsigned char* p = reinterpret_cast(sbuf.data()); - i2d_SSL_SESSION(sess, &p); + CHECK_LT(0, i2d_SSL_SESSION(sess, &p)); args.GetReturnValue().Set(sbuf.ToBuffer().ToLocalChecked()); } From dedbd119c51ae9bae1eccbb92f991b83a7b05613 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 21 Aug 2019 15:12:48 +0200 Subject: [PATCH 5/8] http: fix event listener leak Fixes: https://github.com/nodejs/node/issues/29239 PR-URL: https://github.com/nodejs/node/pull/29245 Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Jiawen Geng Reviewed-By: Matteo Collina --- lib/_http_client.js | 2 ++ test/parallel/test-http-agent-keepalive.js | 11 ++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index cea02eefa30763..a5a869b327c79b 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -502,6 +502,7 @@ function socketOnData(d) { !statusIsInformational(parser.incoming.statusCode)) { socket.removeListener('data', socketOnData); socket.removeListener('end', socketOnEnd); + socket.removeListener('drain', ondrain); freeParser(parser, req, socket); } } @@ -609,6 +610,7 @@ function responseKeepAlive(res, req) { } socket.removeListener('close', socketCloseListener); socket.removeListener('error', socketErrorListener); + socket.removeListener('drain', ondrain); socket.once('error', freeSocketErrorListener); // There are cases where _handle === null. Avoid those. Passing null to // nextTick() will call getDefaultTriggerAsyncId() to retrieve the id. diff --git a/test/parallel/test-http-agent-keepalive.js b/test/parallel/test-http-agent-keepalive.js index 5902c5867968cf..8cfc568b1e76e4 100644 --- a/test/parallel/test-http-agent-keepalive.js +++ b/test/parallel/test-http-agent-keepalive.js @@ -52,7 +52,7 @@ function get(path, callback) { port: server.address().port, agent: agent, path: path - }, callback); + }, callback).on('socket', common.mustCall(checkListeners)); } function checkDataAndSockets(body) { @@ -134,3 +134,12 @@ server.listen(0, common.mustCall(() => { })); })); })); + +// Check for listener leaks when reusing sockets. +function checkListeners(socket) { + assert.strictEqual(socket.listenerCount('data'), 1); + assert.strictEqual(socket.listenerCount('drain'), 1); + assert.strictEqual(socket.listenerCount('error'), 1); + // Sockets have onReadableStreamEnd. + assert.strictEqual(socket.listenerCount('end'), 2); +} From a6abfcb4234192db1b390e75e013533fbd7f9ca1 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 20 Aug 2019 10:16:17 +0200 Subject: [PATCH 6/8] src: remove unused using declarations This commit removes unused using declarations in src/node_contextify.cc. PR-URL: https://github.com/nodejs/node/pull/29222 Reviewed-By: Jiawen Geng Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater Reviewed-By: Luigi Pinca Reviewed-By: Trivikram Kamat --- src/node_contextify.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 69c110d6be2d22..2d30e0b8038ce4 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -49,7 +49,6 @@ using v8::HandleScope; using v8::IndexedPropertyHandlerConfiguration; using v8::Integer; using v8::Isolate; -using v8::Just; using v8::Local; using v8::Maybe; using v8::MaybeLocal; @@ -68,7 +67,6 @@ using v8::ScriptCompiler; using v8::ScriptOrigin; using v8::ScriptOrModule; using v8::String; -using v8::Symbol; using v8::Uint32; using v8::UnboundScript; using v8::Value; From ae0a0e97ba7cf46c3362a238979838cbc3e5deec Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 24 Aug 2019 20:42:11 +0200 Subject: [PATCH 7/8] http: reset parser.incoming when server request is finished This resolves a memory leak for keep-alive connections and does not regress in the way that 779a05d5d1bfe2eeb05386f did by waiting for the incoming request to be finished before releasing the `parser.incoming` object. Refs: https://github.com/nodejs/node/pull/28646 Refs: https://github.com/nodejs/node/pull/29263 Fixes: https://github.com/nodejs/node/issues/9668 PR-URL: https://github.com/nodejs/node/pull/29297 Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina --- lib/_http_server.js | 14 ++++++ .../test-http-server-keepalive-end.js | 15 ++++-- .../test-http-server-keepalive-req-gc.js | 47 +++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-http-server-keepalive-req-gc.js diff --git a/lib/_http_server.js b/lib/_http_server.js index f7b7752b21ae49..58f0973ddd8b02 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -614,6 +614,19 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) { } } +function clearIncoming(req) { + req = req || this; + const parser = req.socket && req.socket.parser; + // Reset the .incoming property so that the request object can be gc'ed. + if (parser && parser.incoming === req) { + if (req.readableEnded) { + parser.incoming = null; + } else { + req.on('end', clearIncoming); + } + } +} + function resOnFinish(req, res, socket, state, server) { // Usually the first incoming element should be our request. it may // be that in the case abortIncoming() was called that the incoming @@ -621,6 +634,7 @@ function resOnFinish(req, res, socket, state, server) { assert(state.incoming.length === 0 || state.incoming[0] === req); state.incoming.shift(); + clearIncoming(req); // If the user never called req.read(), and didn't pipe() or // .resume() or .on('data'), then we call req._dump() so that the diff --git a/test/parallel/test-http-server-keepalive-end.js b/test/parallel/test-http-server-keepalive-end.js index 1e3a44d368a020..8ebdecb97c3d8c 100644 --- a/test/parallel/test-http-server-keepalive-end.js +++ b/test/parallel/test-http-server-keepalive-end.js @@ -1,18 +1,27 @@ 'use strict'; - const common = require('../common'); +const assert = require('assert'); const { createServer } = require('http'); const { connect } = require('net'); +// Make sure that for HTTP keepalive requests, the .on('end') event is emitted +// on the incoming request object, and that the parser instance does not hold +// on to that request object afterwards. + const server = createServer(common.mustCall((req, res) => { - req.on('end', common.mustCall()); + req.on('end', common.mustCall(() => { + const parser = req.socket.parser; + assert.strictEqual(parser.incoming, req); + process.nextTick(() => { + assert.strictEqual(parser.incoming, null); + }); + })); res.end('hello world'); })); server.unref(); server.listen(0, common.mustCall(() => { - const client = connect(server.address().port); const req = [ diff --git a/test/parallel/test-http-server-keepalive-req-gc.js b/test/parallel/test-http-server-keepalive-req-gc.js new file mode 100644 index 00000000000000..aa4bf1a3de9c83 --- /dev/null +++ b/test/parallel/test-http-server-keepalive-req-gc.js @@ -0,0 +1,47 @@ +// Flags: --expose-gc +'use strict'; +const common = require('../common'); +const onGC = require('../common/ongc'); +const { createServer } = require('http'); +const { connect } = require('net'); + +if (common.isWindows) { + // TODO(addaleax): Investigate why and remove the skip. + common.skip('This test is flaky on Windows.'); +} + +// Make sure that for HTTP keepalive requests, the req object can be +// garbage collected once the request is finished. +// Refs: https://github.com/nodejs/node/issues/9668 + +let client; +const server = createServer(common.mustCall((req, res) => { + onGC(req, { ongc: common.mustCall() }); + req.resume(); + req.on('end', common.mustCall(() => { + setImmediate(() => { + client.end(); + global.gc(); + }); + })); + res.end('hello world'); +})); + +server.unref(); + +server.listen(0, common.mustCall(() => { + client = connect(server.address().port); + + const req = [ + 'POST / HTTP/1.1', + `Host: localhost:${server.address().port}`, + 'Connection: keep-alive', + 'Content-Length: 11', + '', + 'hello world', + '' + ].join('\r\n'); + + client.write(req); + client.unref(); +})); From 9cfb3841785cd658bb13369ed642247e089a5533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Mon, 26 Aug 2019 12:00:40 +0200 Subject: [PATCH 8/8] 2019-08-26, Version 12.9.1 (Current) Notable changes: This release fixes two regressions in the http module: * Fixes an event listener leak in the HTTP client. This resulted in lots of warnings during npm/yarn installs. https://github.com/nodejs/node/pull/29245 * Fixes a regression preventing the `'end'` event from being emitted for keepalive requests in case the full body was not parsed. https://github.com/nodejs/node/pull/29263 PR-URL: https://github.com/nodejs/node/pull/29321 --- CHANGELOG.md | 3 ++- doc/changelogs/CHANGELOG_V12.md | 23 +++++++++++++++++++++++ src/node_version.h | 2 +- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9170fa4e3223a6..69e6ecf89047f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,8 @@ release. -12.9.0
+12.9.1
+12.9.0
12.8.1
12.8.0
12.7.0
diff --git a/doc/changelogs/CHANGELOG_V12.md b/doc/changelogs/CHANGELOG_V12.md index 9c7b382931798d..fc93e6a9525df7 100644 --- a/doc/changelogs/CHANGELOG_V12.md +++ b/doc/changelogs/CHANGELOG_V12.md @@ -9,6 +9,7 @@ +12.9.1
12.9.0
12.8.1
12.8.0
@@ -39,6 +40,28 @@ * [io.js](CHANGELOG_IOJS.md) * [Archive](CHANGELOG_ARCHIVE.md) + +## 2019-08-26, Version 12.9.1 (Current), @targos + +### Notable changes + +This release fixes two regressions in the **http** module: + +* Fixes an event listener leak in the HTTP client. This resulted in lots of + warnings during npm/yarn installs (Robert Nagy) [#29245](https://github.com/nodejs/node/pull/29245). +* Fixes a regression preventing the `'end'` event from being emitted for + keepalive requests in case the full body was not parsed (Matteo Collina) [#29263](https://github.com/nodejs/node/pull/29263). + +### Commits + +* [[`3cc8fca299`](https://github.com/nodejs/node/commit/3cc8fca299)] - **crypto**: handle i2d\_SSL\_SESSION() error return (Ben Noordhuis) [#29225](https://github.com/nodejs/node/pull/29225) +* [[`ae0a0e97ba`](https://github.com/nodejs/node/commit/ae0a0e97ba)] - **http**: reset parser.incoming when server request is finished (Anna Henningsen) [#29297](https://github.com/nodejs/node/pull/29297) +* [[`dedbd119c5`](https://github.com/nodejs/node/commit/dedbd119c5)] - **http**: fix event listener leak (Robert Nagy) [#29245](https://github.com/nodejs/node/pull/29245) +* [[`f8f8754d43`](https://github.com/nodejs/node/commit/f8f8754d43)] - ***Revert*** "**http**: reset parser.incoming when server response is finished" (Matteo Collina) [#29263](https://github.com/nodejs/node/pull/29263) +* [[`a6abfcb423`](https://github.com/nodejs/node/commit/a6abfcb423)] - **src**: remove unused using declarations (Daniel Bevenius) [#29222](https://github.com/nodejs/node/pull/29222) +* [[`ff6330a6ac`](https://github.com/nodejs/node/commit/ff6330a6ac)] - **test**: fix 'timeout' typos (cjihrig) [#29234](https://github.com/nodejs/node/pull/29234) +* [[`3c7a1a9090`](https://github.com/nodejs/node/commit/3c7a1a9090)] - **test, http**: add regression test for keepalive 'end' event (Matteo Collina) [#29263](https://github.com/nodejs/node/pull/29263) + ## 2019-08-20, Version 12.9.0 (Current), @targos diff --git a/src/node_version.h b/src/node_version.h index e3158f2d72dfea..2a241bf23d74cc 100644 --- a/src/node_version.h +++ b/src/node_version.h @@ -29,7 +29,7 @@ #define NODE_VERSION_IS_LTS 0 #define NODE_VERSION_LTS_CODENAME "" -#define NODE_VERSION_IS_RELEASE 0 +#define NODE_VERSION_IS_RELEASE 1 #ifndef NODE_STRINGIFY #define NODE_STRINGIFY(n) NODE_STRINGIFY_HELPER(n)