Skip to content

http2: move events to the JSStreamSocket #35772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Trott merged 2 commits into from
Nov 8, 2020
Merged

http2: move events to the JSStreamSocket #35772

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6abb7e5
http2: move events to the JSStreamSocket
mmomtchev Oct 23, 2020
adae822
http2: centralise socket event binding in Http2Session
mmomtchev Oct 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
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: #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 @Trott
mmomtchev authored and Trott committed Nov 8, 2020
commit adae822035b640531044b33892c32a3a3011cf5b
11 changes: 2 additions & 9 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
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