Skip to content
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

http: replace destroySoon with socket.end #36205

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

http: replace destroySoon with socket.end #36205

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dd2e3ed
net: require two events to destroy a socket
mmomtchev Nov 20, 2020
e6699c6
net: replace destroySoon with socket.end()
mmomtchev Nov 21, 2020
8f0e26c
net: close http connections with stream.end()
mmomtchev Nov 22, 2020
f780542
http: lint the PR
mmomtchev Nov 22, 2020
3e1ba08
net: add a unit test for res.end
mmomtchev Nov 23, 2020
1559f5c
http: replace destroySoon with socket.end
mmomtchev Nov 23, 2020
a1b606c
tls: support active closing for TLS sockets
mmomtchev Dec 9, 2020
6038c95
remove useless change
mmomtchev Dec 9, 2020
0dfd1f2
tls: use Writable.end (@ronag)
mmomtchev Dec 9, 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
6 changes: 1 addition & 5 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,11 +709,7 @@ function responseOnEnd() {

if (!req.shouldKeepAlive) {
if (socket.writable) {
mmomtchev marked this conversation as resolved.
Show resolved Hide resolved
debug('AGENT socket.destroySoon()');
if (typeof socket.destroySoon === 'function')
socket.destroySoon();
else
socket.end();
socket.end();
}
assert(!socket.writable);
} else if (req.finished && !this.aborted) {
Expand Down
6 changes: 1 addition & 5 deletions lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,11 +814,7 @@ function resOnFinish(req, res, socket, state, server) {
process.nextTick(emitCloseNT, res);

if (res._last) {
if (typeof socket.destroySoon === 'function') {
socket.destroySoon();
} else {
socket.end();
}
socket.end();
} else if (state.outgoing.length === 0) {
if (server.keepAliveTimeout && typeof socket.setTimeout === 'function') {
socket.setTimeout(server.keepAliveTimeout);
Expand Down
14 changes: 12 additions & 2 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,10 +631,20 @@ Socket.prototype.destroySoon = function() {
if (this.writable)
this.end();

if (this.writableFinished)
if (this.writableFinished && this.readableEnded)
this.destroy();
else
this.once('finish', this.destroy);
this.once('finish', () => {
// If we destroy the socket before we have received an incoming EOF
// the remote might still be sending data that will trigger a RST
// from this host once it is received
// Closing a TCP socket requires two FIN/ACK exchanges, one in every
// direction - this means two events
if (this.readableEnded)
this.destroy();
else
this.once('end', this.destroy);
});
mmomtchev marked this conversation as resolved.
Show resolved Hide resolved
mmomtchev marked this conversation as resolved.
Show resolved Hide resolved
};
mmomtchev marked this conversation as resolved.
Show resolved Hide resolved


Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http-should-keep-alive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const countdown = new Countdown(SHOULD_KEEP_ALIVE.length, () => server.close());
const getCountdownIndex = () => SERVER_RESPONSES.length - countdown.remaining;

const server = net.createServer(function(socket) {
socket.write(SERVER_RESPONSES[getCountdownIndex()]);
socket.end(SERVER_RESPONSES[getCountdownIndex()]);
}).listen(0, function() {
function makeRequest() {
const req = http.get({ port: server.address().port }, function(res) {
Expand Down