Repository navigation
stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams #35904
Description
- Version: v12.16.3+, v14.0.0+, v15.0.0+, master
- Platform: n/a
- Subsystem: stream
What steps will reproduce the bug?
The following code needs a private key and certificate for an HTTPS server.
const { readFileSync } = require('fs');
const tls = require('tls');
const https = require('https');
const net = require('net');
const { Duplex } = require('stream');
class CustomAgent extends https.Agent {
constructor() {
super();
}
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', () => {
console.log('stream end');
}).on('close', () => {
console.log('stream close');
});
return tls.connect({ ...options, socket: stream });
}
}
const httpsServer = https.createServer({
key: readFileSync('https_key.pem'),
cert: readFileSync('https_cert.pem'),
}, (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' },
ca: readFileSync('https_cert.pem'),
}, (res) => {
res.resume();
});
});How often does it reproduce? Is there a required condition?
Every time.
What is the expected behavior?
"stream end" should be displayed
What do you see instead?
Nothing is displayed.
Additional information
This was originally discovered while adding tests for custom HTTP and HTTPS agents for the ssh2 module. I believe the key here is mainly using a custom stream for the socket passed to tls.connect(). However the regression only became evident once I started setting emitClose: false because my agent implementation was relying on the 'close' event being emitted to know when to close the underlying ssh connection and emitting 'close' is expected to be handled by ssh2 (because the protocol has an explicit close message separate from "EOF").
Bisecting reveals ed21d32 as the bad commit. What's happening is that when TLSSocket sees that the socket option is some custom object/stream and not a net.Socket instance, it wraps the stream with a JSStreamSocket. JSStreamSocket happens to have a readStop() implementation that simply pauses the socket. Since ed21d32 added a check for this method's existence, it now gets called when the TLS portion ends, which means the underlying/original stream now stays paused and will never emit 'end' like it used to.
Judging by the added code comments, it appears that this change was made to appease some error on Windows. I think the original error should be solved in a more compatible way.
Activity
[-]stream: regression since v12.16.3[/-][+]stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams[/+]mmomtchev commentedon Nov 3, 2020
I confirm that removing the kludge with the
handle.readStop()fixes your problem, so maybe this issue should be mostly about why is that kludge needed in the first place. My opinion, is that is masks an incorrect Windows behavior. TheECONNRESETis not caused by an extra call toonStreamRead()- there is really a TCP RST packet that gets sent from the server to the client (yeah, it is weird). Callinghandle.readStop()causes the stream to ignore that packet, thus masking the problem. I am not great with Windows sockets, is there someone with good understanding of Windows TCP/IP who can trace what calls does TLS server / libuv make that cause Windows to sent a TCP RST packet?mmomtchev commentedon Nov 3, 2020
@ronag, maybe add an
'error' handler to the https-truncate test until there is a fix/workaround for TLS/libuv? The RST on Windows is absolutely real, it is not an artifact of the stream code.stream: remove kludge masking RST on Windows
mmomtchev commentedon Nov 3, 2020
This is my proposal and maybe create a separate issue for the
RSTon Windowshttp: graceful shutdown on res.end()
win,tcp: make uv_close work like unix
win,tcp: make uv_close work like unix
mmomtchev commentedon Nov 5, 2020
The libuv team is working on a correct solution for
test-https-truncatebut given the looks of it, it won't make it anytime soon in NodeI wonder if the best solution wouldn't be to remove the kludge masking the
RSTpacket on Windows and add an'error'handler totest-https-truncatenow that we know what causes the connection reset and that it can be safely ignored (for that test)There is just one more
RSTon OSX that must be investigated - it seems very similar to the Windows one - caused byres.end()win,tcp: make uv_close work like unix
TLS: improve compliance with shutdown standard
win,tcp: make uv_close work more like unix
6 remaining items
tls: improve handling of shutdown
Mesteery commentedon Feb 17, 2022
Is it fixed?
win,tcp: make uv_close work more like unix
octogonz commentedon Jul 5, 2022
I'm having trouble following the bookkeeping as well. The description for PR #36111 say that it "closes" PR #35946. And PR #35946 had a description saying that it "fixes" this issue #35904. Does that imply that issue #35904 is now fixed? Then why is it still open?
@vtjnash could you clarify?
vtjnash commentedon Jul 5, 2022
From #36111:
The feature requested in the code at the top of this issue has not been implemented in nodejs. I do not know if the nodejs developers want to support this feature (permitting continuing to use using the unencrypted stream after the TLS-encrypted portion is done) or not. Currently nodejs expects to destroy the underlying (unencrypted) stream as soon as the encrypted (TLS) portion ends.
octogonz commentedon Jul 5, 2022
So if it is a feature request, why is it tagged as a "regression"?
vtjnash commentedon Jul 5, 2022
I have no control over tags
octogonz commentedon Jul 5, 2022
👍 Thanks very much for clarifying, very helpful!
win,tcp: make uv_close work more like unix
win,tcp: make uv_close work more like unix
github-actions commentedon Jun 27, 2026
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.
github-actions commentedon Jul 28, 2026
This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.