diff --git a/lib/_http_client.js b/lib/_http_client.js index 885016bf485..f2c0dcc98c7 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -409,11 +409,6 @@ function socketCloseListener() { const req = socket._httpMessage; debug('HTTP socket close'); - // Pull through final chunk, if anything is buffered. - // the ondata function will handle it properly, and this - // is a no-op if no final chunk remains. - socket.read(); - // NOTE: It's important to get parser here, because it could be freed by // the `socketOnData`. const parser = socket.parser; diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js index 0dfee0c9c7b..1fbc67ef51e 100644 --- a/lib/internal/stream_base_commons.js +++ b/lib/internal/stream_base_commons.js @@ -207,6 +207,9 @@ function onStreamRead(arrayBuffer) { return; } + // note: handle should never deliver any more read events after this point. + // (equivalently, it should have called readStop on itself alread). + if (nread !== UV_EOF) { // CallJSOnreadMethod expects the return value to be a buffer. // Ref: https://github.com/nodejs/node/pull/34375 @@ -222,20 +225,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/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index f4850e425f4..25c535d84c5 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -808,6 +808,11 @@ void TLSWrap::ClearOut() { int flags = SSL_get_shutdown(ssl_.get()); if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { eof_ = true; + // FIXME: rfc5246#section-7.2.1 says we should call Destroy + // here, but Shutdown and Destroy in this class also affect the + // underlying stream. That behavior is permitted by the + // standard of the application protocol, but not of nodejs at + // the protocol layer in this file. EmitRead(UV_EOF); } @@ -921,7 +926,7 @@ bool TLSWrap::IsClosing() { int TLSWrap::ReadStart() { Debug(this, "ReadStart()"); - if (underlying_stream() != nullptr) + if (underlying_stream() != nullptr && !eof_) return underlying_stream()->ReadStart(); return 0; } @@ -1080,14 +1085,18 @@ uv_buf_t TLSWrap::OnStreamAlloc(size_t suggested_size) { void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { Debug(this, "Read %zd bytes from underlying stream", nread); + + // Ignore everything after close_notify (rfc5246#section-7.2.1) + // TODO: unregister our interest in read events + if (eof_) + return; + if (nread < 0) { // Error should be emitted only after all data was read ClearOut(); - // Ignore EOF if received close_notify if (nread == UV_EOF) { - if (eof_) - return; + // underlying stream already should have also called ReadStop on itself eof_ = true; } 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 00000000000..9e5a6ba488b --- /dev/null +++ b/test/parallel/test-https-agent-jssocket-close.js @@ -0,0 +1,57 @@ +'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(); + }); +});