Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 4 commits into from

5 participants

@abram

I was seeing an intermittent bug in my app Listening Room when piping an http response into stdin of a process. After digging around I found the bug - response.readable was never getting set to false. So if the response finished while the destination stream was paused, when the destination stream resumed pipe() would call source.resume(), which would throw an error since response.socket didn't exist any more.

The attached test demonstrates the bug - run the test case on the existing code and it will throw an error, apply my patch and it works.

test/simple/test-http-client-pipe.js
((3 lines not shown))
+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');
@TooTallNate Owner

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.

@abram
abram added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ry
ry commented

The test isn't failing for me before the fix: https://gist.github.com/914705

@abram

Interesting. I re-tested with the latest code and found that it was no longer failing for me either. After digging it turns out that joyent/node@2a65d29 fixed the bug that was causing an error in pipe() - now all pipe()-related event handlers get removed as soon as the input ends. Previously, source.resume() could be called after the source emitted 'end', which was a bug.

So my test case no longer fails. Still, it seems that res.readable should be set to false at the time of the 'end' event, since attempting to read from it after that point will cause an error.

I've updated this pull request with a simpler test case that no longer uses pipe(). I've also accidentally pulled a merge into this pull request, which I can't figure out how to fix at the moment. Please ignore that and just look at the diff. I can make a new pull request if necessary.

Finally, I'm wondering if it would be preferable to set readable = false at a lower level (like around line 132), but I don't have a solid enough grasp of the code to be sure of the implications of doing it that way.

@ry
ry commented

Thanks. Landed in 83727a4

@japj

since this has landed, can someone close the pull request?

@koichik koichik closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 31, 2011
  1. @abram
Commits on Apr 1, 2011
  1. @abram
Commits on Apr 12, 2011
  1. @abram
  2. @abram

    Use simpler test case

    abram authored
This page is out of date. Refresh to see the latest.
Showing with 25 additions and 0 deletions.
  1. +1 −0  lib/http.js
  2. +24 −0 test/simple/test-http-response-readable.js
View
1  lib/http.js
@@ -1311,6 +1311,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.
View
24 test/simple/test-http-response-readable.js
@@ -0,0 +1,24 @@
+var common = require('../common');
+var assert = require('assert');
+var http = require('http');
+
+var testServer = new http.Server(function(req, res) {
+ res.writeHead(200);
+ res.write('Hello world');
+ res.end();
+});
+
+testServer.listen(common.PORT);
+
+var request = http.get({
+ host: 'localhost',
+ port: common.PORT,
+ path: '/',
+}, function(response) {
+ assert.equal(response.readable, true, 'response.readable initially true');
+ response.on('end', function() {
+ assert.equal(response.readable, false,
+ 'response.readable set to false after end');
+ testServer.close();
+ });
+});
Something went wrong with that request. Please try again.