v12.9.1 release proposal

CHANGELOG.md

@@ -28,7 +28,8 @@ release.
 </tr>
 <tr>
     <td valign="top">
-<b><a href="doc/changelogs/CHANGELOG_V12.md#12.9.0">12.9.0</a></b><br/>
+<b><a href="doc/changelogs/CHANGELOG_V12.md#12.9.1">12.9.1</a></b><br/>
+<a href="doc/changelogs/CHANGELOG_V12.md#12.9.0">12.9.0</a><br/>
 <a href="doc/changelogs/CHANGELOG_V12.md#12.8.1">12.8.1</a><br/>
 <a href="doc/changelogs/CHANGELOG_V12.md#12.8.0">12.8.0</a><br/>
 <a href="doc/changelogs/CHANGELOG_V12.md#12.7.0">12.7.0</a><br/>

doc/changelogs/CHANGELOG_V12.md

@@ -9,6 +9,7 @@
 </tr>
 <tr>
 <td>
+<a href="#12.9.1">12.9.1</a><br/>
 <a href="#12.9.0">12.9.0</a><br/>
 <a href="#12.8.1">12.8.1</a><br/>
 <a href="#12.8.0">12.8.0</a><br/>
@@ -39,6 +40,28 @@
   * [io.js](CHANGELOG_IOJS.md)
   * [Archive](CHANGELOG_ARCHIVE.md)
 
+<a id="12.9.1"></a>
+## 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)
+
 <a id="12.9.0"></a>
 ## 2019-08-20, Version 12.9.0 (Current), @targos
 

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.

lib/_http_server.js

@@ -614,15 +614,27 @@ 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
   // array will be empty.
   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;
+  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

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;

src/node_crypto.cc

@@ -2317,11 +2317,12 @@ void SSLWrap<Base>::GetSession(const FunctionCallbackInfo<Value>& 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<unsigned char*>(sbuf.data());
-  i2d_SSL_SESSION(sess, &p);
+  CHECK_LT(0, i2d_SSL_SESSION(sess, &p));
   args.GetReturnValue().Set(sbuf.ToBuffer().ToLocalChecked());
 }
 

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)

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;

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);

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);
+}

test/parallel/test-http-server-keepalive-end.js

@@ -0,0 +1,38 @@
+'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(() => {
+    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 = [
+    'POST / HTTP/1.1',
+    `Host: localhost:${server.address().port}`,
+    'Connection: keep-alive',
+    'Content-Length: 11',
+    '',
+    'hello world',
+    ''
+  ].join('\r\n');
+
+  client.end(req);
+}));

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();
+}));