Skip to content

Commit

Permalink
stream: remove kludge masking RST on Windows
Browse files Browse the repository at this point in the history
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: nodejs#35904
  • Loading branch information
mmomtchev committed Nov 3, 2020
1 parent 1bc1f84 commit 1d26439
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 15 deletions.
14 changes: 0 additions & 14 deletions lib/internal/stream_base_commons.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
55 changes: 55 additions & 0 deletions test/parallel/test-https-agent-jssocket-close.js
Original file line number Diff line number Diff line change
@@ -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();
});
});
4 changes: 3 additions & 1 deletion test/parallel/test-https-truncate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 1d26439

Please sign in to comment.