From 6abb7e594417ce8bf539fcfc896b3ab3b2c6b5d5 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Fri, 23 Oct 2020 13:16:15 +0200 Subject: [PATCH 1/2] http2: move events to the JSStreamSocket When using a JSStreamSocket, the HTTP2Session constructor will replace the socket object http2 events should be attached to the JSStreamSocket object because the http2 session handle lives there Fixes: https://github.com/nodejs/node/issues/35695 PR-URL: https://github.com/nodejs/node/pull/35772 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott Reviewed-By: James M Snell Reviewed-By: Ricky Zhou <0x19951125@gmail.com> --- lib/internal/http2/core.js | 9 ++- .../test-http2-client-jsstream-destroy.js | 55 +++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-http2-client-jsstream-destroy.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 7bd30eeaf10a10..dd7501c40b3ea3 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -3132,11 +3132,14 @@ function connect(authority, options, listener) { } } - socket.on('error', socketOnError); - socket.on('close', socketOnClose); - const session = new ClientHttp2Session(options, socket); + // ClientHttp2Session may create a new socket object + // when socket is a JSSocket (socket != kSocket) + // https://github.com/nodejs/node/issues/35695 + session[kSocket].on('error', socketOnError); + session[kSocket].on('close', socketOnClose); + session[kAuthority] = `${options.servername || host}:${port}`; session[kProtocol] = protocol; diff --git a/test/parallel/test-http2-client-jsstream-destroy.js b/test/parallel/test-http2-client-jsstream-destroy.js new file mode 100644 index 00000000000000..f2f0669170c4bc --- /dev/null +++ b/test/parallel/test-http2-client-jsstream-destroy.js @@ -0,0 +1,55 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const h2 = require('http2'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); +const { Duplex } = require('stream'); + +const server = h2.createSecureServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') +}); + +class JSSocket extends Duplex { + constructor(socket) { + super({ emitClose: true }); + socket.on('close', () => this.destroy()); + socket.on('data', (data) => this.push(data)); + this.socket = socket; + } + + _write(data, encoding, callback) { + this.socket.write(data, encoding, callback); + } + + _read(size) { + } + + _final(cb) { + cb(); + } +} + +server.listen(0, common.mustCall(function() { + const socket = tls.connect({ + rejectUnauthorized: false, + host: 'localhost', + port: this.address().port, + ALPNProtocols: ['h2'] + }, () => { + const proxy = new JSSocket(socket); + const client = h2.connect(`https://localhost:${this.address().port}`, { + createConnection: () => proxy + }); + const req = client.request(); + + setTimeout(() => socket.destroy(), 200); + setTimeout(() => client.close(), 400); + setTimeout(() => server.close(), 600); + + req.on('close', common.mustCall(() => { })); + }); +})); From adae822035b640531044b33892c32a3a3011cf5b Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Sat, 24 Oct 2020 15:50:20 +0200 Subject: [PATCH 2/2] http2: centralise socket event binding in Http2Session Move the socket event binding to the HTTP2Session constructor so that an error event could be delivered should the constructor fail Ref: https://github.com/nodejs/node/pull/35772 PR-URL: https://github.com/nodejs/node/pull/35772 Fixes: https://github.com/nodejs/node/issues/35695 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott Reviewed-By: James M Snell Reviewed-By: Ricky Zhou <0x19951125@gmail.com> --- lib/internal/http2/core.js | 11 ++--------- test/parallel/test-http2-client-jsstream-destroy.js | 6 +++--- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index dd7501c40b3ea3..b585d61cd1db92 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1129,6 +1129,8 @@ class Http2Session extends EventEmitter { if (!socket._handle || !socket._handle.isStreamBase) { socket = new JSStreamSocket(socket); } + socket.on('error', socketOnError); + socket.on('close', socketOnClose); // No validation is performed on the input parameters because this // constructor is not exported directly for users. @@ -2909,9 +2911,6 @@ function connectionListener(socket) { return; } - socket.on('error', socketOnError); - socket.on('close', socketOnClose); - // Set up the Session const session = new ServerHttp2Session(options, socket, this); @@ -3134,12 +3133,6 @@ function connect(authority, options, listener) { const session = new ClientHttp2Session(options, socket); - // ClientHttp2Session may create a new socket object - // when socket is a JSSocket (socket != kSocket) - // https://github.com/nodejs/node/issues/35695 - session[kSocket].on('error', socketOnError); - session[kSocket].on('close', socketOnClose); - session[kAuthority] = `${options.servername || host}:${port}`; session[kProtocol] = protocol; diff --git a/test/parallel/test-http2-client-jsstream-destroy.js b/test/parallel/test-http2-client-jsstream-destroy.js index f2f0669170c4bc..f881eac47aa809 100644 --- a/test/parallel/test-http2-client-jsstream-destroy.js +++ b/test/parallel/test-http2-client-jsstream-destroy.js @@ -46,9 +46,9 @@ server.listen(0, common.mustCall(function() { }); const req = client.request(); - setTimeout(() => socket.destroy(), 200); - setTimeout(() => client.close(), 400); - setTimeout(() => server.close(), 600); + setTimeout(() => socket.destroy(), common.platformTimeout(100)); + setTimeout(() => client.close(), common.platformTimeout(200)); + setTimeout(() => server.close(), common.platformTimeout(300)); req.on('close', common.mustCall(() => { })); });