This repository has been archived by the owner. 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

Conversation

Projects
None yet
5 participants

abram commented Mar 31, 2011

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
+ this.output += buffer.toString();
+ console.log('wrote data.');
+ var self = this;
+ this.emit('pause');
@TooTallNate

TooTallNate Apr 1, 2011

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 Apr 1, 2011

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.

ry commented Apr 12, 2011

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

abram commented Apr 12, 2011

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 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 commented Apr 12, 2011

Thanks. Landed in 83727a4

coolaj86 pushed a commit that referenced this pull request Apr 15, 2011

japj commented Jul 10, 2011

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

@koichik koichik closed this Jul 10, 2011

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.