Skip to content

Commit

Permalink
http2: centralise socket event binding in Http2Session
Browse files Browse the repository at this point in the history
Move the socket event binding to the
HTTP2Session constructor so that an error
event could be delivered should the
constructor fail

Ref: #35772

PR-URL: #35772
Fixes: #35695
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
  • Loading branch information
Momtchil Momtchev authored and Trott committed Nov 8, 2020
1 parent 6abb7e5 commit adae822
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 12 deletions.
11 changes: 2 additions & 9 deletions lib/internal/http2/core.js
Expand Up @@ -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.
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;

Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-http2-client-jsstream-destroy.js
Expand Up @@ -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(() => { }));
});
Expand Down

0 comments on commit adae822

Please sign in to comment.