From 1d2643963ac6fe971e0b82f148571cd6d65be5f9 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Tue, 3 Nov 2020 16:57:57 +0100 Subject: [PATCH 1/3] stream: remove kludge masking RST on Windows Remove the kludge that masks the TCP RST on Windows on test-https-truncate That RST is very real, originates from the server and should be investigated Fixes: https://github.com/nodejs/node/issues/35904 --- lib/internal/stream_base_commons.js | 14 ----- .../test-https-agent-jssocket-close.js | 55 +++++++++++++++++++ test/parallel/test-https-truncate.js | 4 +- 3 files changed, 58 insertions(+), 15 deletions(-) create mode 100644 test/parallel/test-https-agent-jssocket-close.js diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js index 0dfee0c9c7b619..1e3ba3a612c113 100644 --- a/lib/internal/stream_base_commons.js +++ b/lib/internal/stream_base_commons.js @@ -222,20 +222,6 @@ function onStreamRead(arrayBuffer) { if (stream[kMaybeDestroy]) stream.on('end', stream[kMaybeDestroy]); - // TODO(ronag): Without this `readStop`, `onStreamRead` - // will be called once more (i.e. after Readable.ended) - // on Windows causing a ECONNRESET, failing the - // test-https-truncate test. - if (handle.readStop) { - const err = handle.readStop(); - if (err) { - // CallJSOnreadMethod expects the return value to be a buffer. - // Ref: https://github.com/nodejs/node/pull/34375 - stream.destroy(errnoException(err, 'read')); - return; - } - } - // Push a null to signal the end of data. // Do it before `maybeDestroy` for correct order of events: // `end` -> `close` diff --git a/test/parallel/test-https-agent-jssocket-close.js b/test/parallel/test-https-agent-jssocket-close.js new file mode 100644 index 00000000000000..78c1076de3a3ab --- /dev/null +++ b/test/parallel/test-https-agent-jssocket-close.js @@ -0,0 +1,55 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); +const https = require('https'); +const key = fixtures.readKey('agent1-key.pem'); +const cert = fixtures.readKey('agent1-cert.pem'); +const net = require('net'); +const { Duplex } = require('stream'); + +class CustomAgent extends https.Agent { + createConnection(options, cb) { + const realSocket = net.createConnection(options); + const stream = new Duplex({ + emitClose: false, + read(n) { + (function retry() { + const data = realSocket.read(); + if (data === null) + return realSocket.once('readable', retry); + stream.push(data); + })(); + }, + write(chunk, enc, callback) { + realSocket.write(chunk, enc, callback); + }, + }); + realSocket.on('end', () => stream.push(null)); + + stream.on('end', common.mustCall()); + return tls.connect({ ...options, socket: stream }); + } +} + +const httpsServer = https.createServer({ + key, + cert, +}, (req, res) => { + httpsServer.close(); + res.end('hello world!'); +}); +httpsServer.listen(0, 'localhost', () => { + const agent = new CustomAgent(); + https.get({ + host: 'localhost', + port: httpsServer.address().port, + agent, + headers: { Connection: 'close' }, + rejectUnauthorized: false + }, (res) => { + res.resume(); + }); +}); diff --git a/test/parallel/test-https-truncate.js b/test/parallel/test-https-truncate.js index beed36cd7c0819..3d70a56cc0e1d4 100644 --- a/test/parallel/test-https-truncate.js +++ b/test/parallel/test-https-truncate.js @@ -50,11 +50,13 @@ function httpsTest() { const opts = { port: this.address().port, rejectUnauthorized: false }; https.get(opts).on('response', function(res) { test(res); + }).on('error', () => { + // TODO: investigate why the server closes with a TCP RST on Windows + // https://github.com/nodejs/node/issues/35904 }); }); } - const test = common.mustCall(function(res) { res.on('end', common.mustCall(function() { assert.strictEqual(res.readableLength, 0); From 07219ab2f544ae10417dbc6f377129bbbf39d704 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Wed, 4 Nov 2020 12:27:13 +0100 Subject: [PATCH 2/3] http: graceful shutdown on res.end() On Windows and OSX, the HTTP server will often make the OS send a TCP RST packet by shutting down the socket before all incoming data has been read. Normally, libuv should take care of this (?), but it does not on Windows and OSX Refs: https://github.com/nodejs/node/issues/35904 --- lib/_http_server.js | 5 ++++- test/parallel/test-https-truncate.js | 5 +---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 7343d5f52791b9..fa5281f7e9e880 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -815,7 +815,10 @@ function resOnFinish(req, res, socket, state, server) { if (res._last) { if (typeof socket.destroySoon === 'function') { - socket.destroySoon(); + if (socket.readableEnded) + socket.destroySoon(); + else + socket.on('end', socket.destroySoon); } else { socket.end(); } diff --git a/test/parallel/test-https-truncate.js b/test/parallel/test-https-truncate.js index 3d70a56cc0e1d4..40a8c608ab6d5d 100644 --- a/test/parallel/test-https-truncate.js +++ b/test/parallel/test-https-truncate.js @@ -48,11 +48,8 @@ function httpsTest() { server.listen(0, function() { const opts = { port: this.address().port, rejectUnauthorized: false }; - https.get(opts).on('response', function(res) { + https.get(opts).on('response', function (res) { test(res); - }).on('error', () => { - // TODO: investigate why the server closes with a TCP RST on Windows - // https://github.com/nodejs/node/issues/35904 }); }); } From 87baa7551854163a343d96dbfd05bb32122697f6 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Wed, 4 Nov 2020 15:16:14 +0100 Subject: [PATCH 3/3] libuv: Delay closing the socket when read is pending On Windows, do not skip delaying the closesocket when the socket is not shared with another process - I don't see any reason which will allow this optimization Refs: https://github.com/nodejs/node/pull/35946 Refs: https://github.com/libuv/libuv/issues/3034 --- deps/uv/src/win/tcp.c | 6 +----- lib/_http_server.js | 5 +---- test/parallel/test-https-truncate.js | 2 +- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/deps/uv/src/win/tcp.c b/deps/uv/src/win/tcp.c index 0dcaa97df70821..f5ff14d0eb12dd 100644 --- a/deps/uv/src/win/tcp.c +++ b/deps/uv/src/win/tcp.c @@ -1429,11 +1429,7 @@ void uv_tcp_close(uv_loop_t* loop, uv_tcp_t* tcp) { if (tcp->flags & UV_HANDLE_READ_PENDING) { /* In order for winsock to do a graceful close there must not be any any * pending reads, or the socket must be shut down for writing */ - if (!(tcp->flags & UV_HANDLE_SHARED_TCP_SOCKET)) { - /* Just do shutdown on non-shared sockets, which ensures graceful close. */ - shutdown(tcp->socket, SD_SEND); - - } else if (uv_tcp_try_cancel_io(tcp) == 0) { + if (uv_tcp_try_cancel_io(tcp) == 0) { /* In case of a shared socket, we try to cancel all outstanding I/O,. If * that works, don't close the socket yet - wait for the read req to * return and close the socket in uv_tcp_endgame. */ diff --git a/lib/_http_server.js b/lib/_http_server.js index fa5281f7e9e880..7343d5f52791b9 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -815,10 +815,7 @@ function resOnFinish(req, res, socket, state, server) { if (res._last) { if (typeof socket.destroySoon === 'function') { - if (socket.readableEnded) - socket.destroySoon(); - else - socket.on('end', socket.destroySoon); + socket.destroySoon(); } else { socket.end(); } diff --git a/test/parallel/test-https-truncate.js b/test/parallel/test-https-truncate.js index 40a8c608ab6d5d..a35ce8ba74d7a4 100644 --- a/test/parallel/test-https-truncate.js +++ b/test/parallel/test-https-truncate.js @@ -48,7 +48,7 @@ function httpsTest() { server.listen(0, function() { const opts = { port: this.address().port, rejectUnauthorized: false }; - https.get(opts).on('response', function (res) { + https.get(opts).on('response', function(res) { test(res); }); });