Skip to content

http: replace destroySoon with socket.end #36205

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

Closed
mmomtchev wants to merge 9 commits into from
Closed

http: replace destroySoon with socket.end #36205

Show file tree
Hide file tree
Changes from 6 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
8 changes: 1 addition & 7 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,13 +708,7 @@ function responseOnEnd() {
req._ended = true;

if (!req.shouldKeepAlive) {
if (socket.writable) {
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) {
// We can assume `req.finished` means all data has been written since:
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
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
31 changes: 31 additions & 0 deletions test/parallel/test-https-end-rst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';
const common = require('../common');

const http = require('http');
const net = require('net');

const data = 'hello';
const server = http.createServer({}, common.mustCallAtLeast((req, res) => {
res.setHeader('content-length', data.length);
res.end(data);
}, 2));

server.listen(0, function() {
const options = {
host: '127.0.0.1',
port: server.address().port,
allowHalfOpen: true
};
const socket = net.connect(options, common.mustCall(() => {
socket.on('ready', common.mustCall(() => {
socket.write('GET /\n\n');
socket.once('data', common.mustCall(() => {
socket.write('GET /\n\n');
setTimeout(common.mustCall(() => {
socket.destroy();
server.close();
}), common.platformTimeout(10));
Copy link
Copy Markdown
Member

@mcollina mcollina Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the timeout needed? Could we use some event on the other side?

Copy link
Copy Markdown
Contributor Author

@mmomtchev mmomtchev Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You won't get anything on the second request - the server has already closed its end
The previous version will send a RST
The new one will silently swallow your request

Copy link
Copy Markdown
Contributor Author

@mmomtchev mmomtchev Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina The more I look at this, the more I am starting to question this behavior
Because now it is correct from the socket standpoint, but not from the protocol
Normally, an HTTP/1.1 connection with Connection: close should have a mandatory simultaneous close
In this case, force-closing and sending back a RST if the other end continues writing is the correct behavior

We got the TLS right, but not HTTP/1.1

Copy link
Copy Markdown
Contributor Author

@mmomtchev mmomtchev Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for this to work, we have to correctly identify the exact protocol version
Then enforce a mandatory simultaneous close in HTTP/1.1 and TLS/1.0
And drain the input side on HTTP/1.0 and TLS/1.1+

Copy link
Copy Markdown
Contributor Author

@mmomtchev mmomtchev Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax @lpinca @jasnell
What do you think?

Copy link
Copy Markdown
Member

@lpinca lpinca Nov 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the test, I think it is possible to remove the timeout by listening to the 'finish' event of the response socket (after calling response.end() to ensure that the listener will be added after the one that currently destroys the socket). When that event is emitted write again from the client socket.

Copy link
Copy Markdown
Contributor Author

@mmomtchev mmomtchev Nov 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpinca I have no response object, I am doing manual HTTP with net?

I fixed the gist and I ran it with a Node without the readStop kludge in stream_base_commons.js - it was still not very reliable detecting the RST until I added a third write - ie. if you call socket.write() to send some data and that data gets a RST in return you won't get an error until you read - which you won't since the Readable has ended - or call another write. Since this is very edge behavior anyway, I won't call it a bug.

The test is still very random - usually naked HTTP servers (ie Apache) will read after FIN while the big load-balancers of the top sites won't

Copy link
Copy Markdown
Member

@lpinca lpinca Nov 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no response object, I am doing manual HTTP with net?

You have it on the server. Your server is an HTTP server, your client is a plain net.Socket. Basically wait for the 'finish' event to be emitted on the response, then wait for the 'finish' event on the response socket (if it is not finished yet) and when it is emitted write again on the client socket.

Copy link
Copy Markdown
Contributor Author

@mmomtchev mmomtchev Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is that Node ignores SIGPIPE on Linux - normally if you call write (the kernel call) in non-blocking mode, it will just schedule your write and return. This is what libuv does too. Then later, if this write gets a RST in return - the only way to notify you would be a SIGPIPE - if you don't do any other calls on this descriptor and you have ignored SIGPIPE you won't get an error. But once again, this is a very edge behavior, I don't think it is worth it to try to fix it.

Anyway, this confirms that HTTP's behavior, when it comes to closing the connection, is undefined and both implementations exist in the wild

Copy link
Copy Markdown
Contributor Author

@mmomtchev mmomtchev Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the extra write the gist should work even with a stock Node

}));
}));
}));
});