Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

In http.Agent, response.readable is never set to false, causing bugs in response.pipe() #867

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions lib/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -1305,6 +1305,7 @@ Agent.prototype._establishNewConnection = function() {

res.addListener('end', function() {
debug('AGENT request complete');
res.readable = false;
// For the moment we reconnect for every request. FIXME!
// All that should be required for keep-alive is to not reconnect,
// but outgoingFlush instead.
Expand Down
60 changes: 60 additions & 0 deletions test/simple/test-http-client-pipe.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
var common = require('../common');
var assert = require('assert');
var http = require('http');
var stream = require('stream');
var util = require('util');

function SlowStream() {
stream.Stream.call(this);
this.writable = true;
this.output = '';
}
util.inherits(SlowStream, stream.Stream);

SlowStream.prototype.write = function(buffer) {
this.output += buffer.toString();
console.log('wrote data.');
var self = this;
this.emit('pause');

Choose a reason for hiding this comment

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

You shouldn't need to emit pause on a writable stream. Returning false from the write function will in turn make the read stream call pause on itself. See the Stream#pipe() source to see what I'm talking about.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. Updated to return false instead of emitting 'pause'. This doesn't change the behavior of the test case in demonstrating the bug.

setTimeout(function() {
self.emit('drain');
}, 100);
};

SlowStream.prototype.end = function() {
testServer.close();
};

var chunks = ['hello ', 'world'];

var testServer = new http.Server(function(req, res) {
res.writeHead(200);
res.write(chunks[0]);
process.nextTick(function() {
res.write(chunks[1]);
process.nextTick(function() {
res.end();
});
});
});

testServer.listen(common.PORT);

var outputStream = new SlowStream();

var request = http.get({
host: 'localhost',
port: common.PORT,
path: '/',
}, function(response) {
response.on('data', function() { console.log('got some data'); });
// If response.readable isn't being properly set to false, this will
// cause an error, because the final time .resume() is called on the
// response the underlying socket will already be closed.
response.pipe(outputStream);
});
request.end();

process.addListener('exit', function() {
assert.equal(outputStream.output, chunks.join(''));
});