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
mmomtchev authored and BethGriggs committed Dec 9, 2020
1 parent dc2935d commit c6900dd
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
Original file line number Diff line number Diff line change
Expand Up @@ -1120,6 +1120,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 @@ -2887,9 +2889,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 @@ -3100,12 +3099,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
Original file line number Diff line number Diff line change
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 c6900dd

Please sign in to comment.